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

DynamicObjects can now access fields by public name. #1571

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

clementetb
Copy link
Contributor

Fixes CI

@clementetb clementetb self-assigned this Nov 14, 2023
Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

This doesn't look right. How can we know the @PersistedName mapping when there is not necessarily a schema definition?

@clementetb
Copy link
Contributor Author

Then it won't provide access to a public name because there won't be any.

Right now this behavior is a by-product of building the dynamic realm out of a typed realm config. It has a schema definition, and thus has public names available.

I don't think this should be an issue as it only affects an internal function, and even if it was public we would allow users to access data using a public name if required.

@rorbech
Copy link
Contributor

rorbech commented Nov 14, 2023

I don't think it is the right way to go. This indeed looks like a public change and we have tried not to leverage the typed schema information even if we could. But maybe it is as broader discussion on what the full dynamic API should support. Lets discuss in todays meeting.

Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

Great. One minor comment.

assertFailsWithMessage<IllegalArgumentException>("Schema for type 'AlternativePersistedNameSample' doesn't contain a property named 'publicNameStringField'") {
dynamicSample.getValue("publicNameStringField")
}
assertEquals("Realm", dynamicSample.getValue("publicNameStringField"))
Copy link
Contributor

@rorbech rorbech Nov 15, 2023

Choose a reason for hiding this comment

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

It would probably be appropriate with a comment here explaining that this is only due to the nature of asDynamicRealm that the public name is available here.

@clementetb clementetb merged commit 30c3bfd into releases Nov 15, 2023
2 checks passed
@clementetb clementetb deleted the ct/fix-persistedname-failing-testcase branch November 15, 2023 09:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants