Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only nodes from a single language are deserialized #61

Closed
woppa684 opened this issue Sep 4, 2023 · 10 comments
Closed

Only nodes from a single language are deserialized #61

woppa684 opened this issue Sep 4, 2023 · 10 comments

Comments

@woppa684
Copy link
Contributor

woppa684 commented Sep 4, 2023

A model containing nodes from different languages cannot be deserialized.

"languages": [
    {"version":"9","key":"NWQ4ZDZmZTUtODExZS00NTBhLWFhYjEtZjA2YzBkOWJhODk2"},
    {"version":"1","key":"LIonCore-builtins"},
    {"version":"0","key":"OTJkMmVhMTYtNWE0Mi00ZmRmLWE2NzYtYzc2MDRlZmUzNTA0"},
    {"version":"2","key":"LIonCore-builtins"}
]

I have a model that contains 4 languages in its languages property., however, I can only specify a single language when calling deserializeModel and nodes from any of the other languages (apart from the builtins) throw an error like:

Error: can't deserialize a node having concept with ID 'OTJkMmVhMTYtNWE0Mi00ZmRmLWE2NzYtYzc2MDRlZmUzNTA0LzI1NTcwNzQ0NDI5MjIzODA4OTc'
dslmeinte added a commit that referenced this issue Sep 4, 2023
@dslmeinte
Copy link
Contributor

Good catch! I've made a preliminary fix in branch fix/multi-language-models, but that raises questions about the relation between languages (as instances of the M3 Language metaconcept) and their APIs. It's unrealistic to assume that all languages have the same API. This also raises questions about language composition.

Could you maybe contribute a multi-lingual test case?

@woppa684
Copy link
Contributor Author

woppa684 commented Sep 4, 2023

I created a fairly minimal failing test here #62

@dslmeinte
Copy link
Contributor

Ha, the serialization is already wrong: it's missing nodes and properties, for some reason...

dslmeinte added a commit that referenced this issue Sep 4, 2023
@dslmeinte
Copy link
Contributor

OK, got things working again: see e7a727d

But we still have to rethink the relation language ⇆ API...

@woppa684
Copy link
Contributor Author

woppa684 commented Sep 5, 2023

I think it would make sense to have a single API but make the methods "language-aware". For deserialization it is already easy, since the language is in the meta pointer so it can easily be passed to the API funtions. For serialization the language probably needs to be added to a language-specific BaseNode that extends the BaseNode used in the API? At least, if you want to follow the current way it works in the examples.

@dslmeinte
Copy link
Contributor

I agree: one API but one that's aware of which language a certain Node comes from (when serializing). I was thinking of adding a function to the ModelAPI type that deduces the Language before deducing the Concept. That should make it easy to compose languages in specific ways.

I'm also thinking of splitting the ModelAPI type in a ReadModelAPI and a WriteModelAPI piece - thoughts?

@woppa684
Copy link
Contributor Author

woppa684 commented Sep 5, 2023

Yeah, splitting would definitely make sense to me. It makes it also more clear what (for example) the difference is between encodingOf and enumLiteralFrom .

@dslmeinte
Copy link
Contributor

See commit 5431121 for what the splitting might look like.

@dslmeinte
Copy link
Contributor

And commit 611df30 for what the extended API could look like.

dslmeinte added a commit that referenced this issue Sep 13, 2023
dslmeinte added a commit that referenced this issue Sep 14, 2023
dslmeinte added a commit that referenced this issue Sep 14, 2023
dslmeinte added a commit that referenced this issue Sep 15, 2023
* Multi language deserializer test (#62)

* Minimal example of failing multi-lang deserialziing

* Small typo

---------

Co-authored-by: Roel van Bakel <[email protected]>

* fix issue #61 (preliminarily!)

* wrangle unit test to work with the preliminary fix

* (fix code after rebase)

* process Arsene's comments

  + reorganize models/ into "languages" vs. "instances"
  + reorganize src-test to align with models/

---------

Co-authored-by: Roel van Bakel <[email protected]>
Co-authored-by: Roel van Bakel <[email protected]>
@dslmeinte
Copy link
Contributor

Fixed with PR #63

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants