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

JsonSerializer: Support references to other documents #1254

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

spoenemann
Copy link
Contributor

The current implementation of the JsonSerializer assumes that all cross-references point to nodes in the same document. This change adds support for references to other documents by including the document URI in the $ref string.

Copy link
Contributor

@sailingKieler sailingKieler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
Just one question: There's no way of including a custom fileURI handler in the deseralization (reviving) process, correct? Shouldn't we allow that, too? Just to be symmetric

if (this.ignoreProperties.has(key)) {
return undefined;
} else if (isReference(value)) {
const refValue = value.ref;
const $refText = refText ? value.$refText : undefined;
if (refValue) {
const targetDocument = getDocument(refValue);
let targetUri = '';
if (this.currentDocument && !UriUtils.equals(this.currentDocument.uri, targetDocument.uri)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe more of curiosity: Do you see a case this.currentDocument !== targetDocument is not sufficient/correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, rethinking this, not really. In general, equal URIs implies equal documents.

@spoenemann
Copy link
Contributor Author

Just one question: There's no way of including a custom fileURI handler in the deseralization (reviving) process, correct? Shouldn't we allow that, too? Just to be symmetric

Yes that makes sense, I forgot that.

Copy link
Contributor

@sailingKieler sailingKieler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect 👌

@spoenemann spoenemann merged commit 7066ac4 into eclipse-langium:main Nov 9, 2023
@spoenemann spoenemann deleted the serialize-uris branch November 9, 2023 09:47
@cdietrich
Copy link
Contributor

@spoenemann i wonder if we can make this configurable

@spoenemann
Copy link
Contributor Author

Yes you can pass in a uriConverter to configure it.

@cdietrich
Copy link
Contributor

thx. was a test data error anyway. works as expected if that one is fixed

@spoenemann spoenemann added this to the v3.0.0 milestone Jun 17, 2024
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

Successfully merging this pull request may close these issues.

3 participants