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

Document serialization inconsistency/danger #989

Open
bjornharrtell opened this issue Oct 27, 2024 · 2 comments
Open

Document serialization inconsistency/danger #989

bjornharrtell opened this issue Oct 27, 2024 · 2 comments

Comments

@bjornharrtell
Copy link
Contributor

I note that JsonProperty (Newtonsoft annotation) is used for most properties, but not all, with a snake_case name. This has been and still is adhered to in Cosmos DB API because it still uses Newtonsoft. I think this is dangerous for serveral reasons.

Also, it's not been consistent. For example property KeyExternalValidityInMonths for Track entity did not have JsonProperty in version 1.1.0 but it was introduced later, but I'm not sure about the consequences.

I noted this when testing the PostgreSql repositores which underlying implementation uses STJ JsonProperty is ignored.

I would recommend a long term plan to remove JsonProperty and treat model properties as the source of truth and have explicit serialization configuration for the repositories to avoid changes due to changes in depencendies. This will, however, require automated schema migration logic to be implemented in a non-breaking way.

@Revsgaard
Copy link
Member

You are right, I'm just a little afraid of changing it now :)

The MongoDbJsonPropertyConvention makes Mongo DB support JsonProperty and read the snake_case name.

The data is lost if the JsonProperty with snake_case is added later. The KeyExternalValidityInMonths property in Track has fortunately been phased out.

I'm afraid some of the property names and snake_case names to not match 100%.

@bjornharrtell
Copy link
Contributor Author

Yeah this will be very difficult to do something about without breakage. Likely it would require a full data migration step when upgrading.

I'm not sure how to reasonably make the PG repositories respect JsonProperty. But mabye that is acceptable.. as long as you do not change property names without also migrating JsonProperty alias.

Just have to watch out for when Newtonsoft is phased out elsewhere.

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