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

Update JOptions configuration (Lombiq Technologies: OCORE-149) #15534

Merged
merged 6 commits into from
Mar 21, 2024

Conversation

sarahelsaig
Copy link
Contributor

Fixes #15533

@MikeAlhayek
Copy link
Member

MikeAlhayek commented Mar 18, 2024

This fix is good and will result in one less breaking change. I am not sure if there is a negative impact from this change. @sebastienros any thoughts if there is a downside from taking this change?

@MikeAlhayek
Copy link
Member

MikeAlhayek commented Mar 18, 2024

@sarahelsaig on a second note, I think this change should not be part of Base (maybe we do, but probably not). It should only be part of the ContentSerializerOptions

https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore/OrchardCore.Abstractions/Extensions/ContentSerializerJsonOptionsConfiguration.cs

Not that ContentSerializerOptions is the only options that impact serializing and deserializing object in YesSql.

@sarahelsaig
Copy link
Contributor Author

@sarahelsaig on a second note, I think this change should not be part of Base (maybe we do, but probably not). It should only be part of the ContentSerializerOptions

https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore/OrchardCore.Abstractions/Extensions/ContentSerializerJsonOptionsConfiguration.cs

Not that ContentSerializerOptions is the only options that impact serializing and deserializing object in YesSql.

Won't it be bad if Base has a different ReferenceHandler than ContentSerializerOptions?

@MikeAlhayek
Copy link
Member

Won't it be bad if Base has a different ReferenceHandler than ContentSerializerOptions?

I don't think so. YesSQL can has different options. for example, in the YesSQL we add converters for different types.

@sarahelsaig
Copy link
Contributor Author

@MikeAlhayek I've made the change you suggested and unfortunately it won't work. I don't think the problem is only related to YesSQL.

I guess your approach works as long as the original structure (i.e. the ContentElement.Elements) is maintained, but it breaks if you use methods which clear that element list such as ContentElement.Apply(), ContentElement.Apply<TPart>() or ContentItem.Merge(). At that point trying to access a child item (e.g. using the ContentItem.As<TPart>()) causes a de-serialization which now only uses the JOptions.Default. This is too fragile, I'm not sure if anything besides read-only access is safe.

So I think this config won't be usable unless we put it into JOptions.Base after all.

@Piedone Piedone changed the title Update JOptions configuration Update JOptions configuration (Lombiq Technologies: OCORE-149) Mar 19, 2024
@MikeAlhayek MikeAlhayek merged commit 7326e8c into OrchardCMS:main Mar 21, 2024
4 checks passed
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.

Content parts with get-only collections can't retain their values (JOptions configuration)
3 participants