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

Is it possible to remove the readonly from JOptions.Default? #16506

Closed
hyzx86 opened this issue Jul 30, 2024 · 10 comments
Closed

Is it possible to remove the readonly from JOptions.Default? #16506

hyzx86 opened this issue Jul 30, 2024 · 10 comments

Comments

@hyzx86
Copy link
Contributor

hyzx86 commented Jul 30, 2024

I would like to make some custom changes to JOptions.Default before run AddOrchardCms,

for example:

            JOptions.Default.Converters.Add(new TimeSpanJsonConverter());
            JOptions.Default.Converters.Add(new DateTimeJsonConverter());
            JOptions.Default.Converters.Add(new DynamicFilterInfoConverter());
            //JOptions.Default.Converters.Add(new ExpandoObjectConverter());
            JOptions.Default.Converters.Add(new StringConverter());

            JOptions.Default.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping;
            JOptions.Default.DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull;

But because it's read-only, it gets the following error.

 System.TypeInitializationException : The type initializer for 'OrchardCore.Tests.Apis.Context.SiteContext' threw an exception.
---- System.InvalidOperationException : This JsonSerializerOptions instance is read-only or has already been used in serialization or deserialization.

Inevitably custom code will contain types that need to implement custom serialisation, but readonly restricts this.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jul 30, 2024

Hi @MikeAlhayek what do you think of this question please?

@gvkries
Copy link
Contributor

gvkries commented Jul 30, 2024

IIRC, in STJ, the JsonSerializerOptions are automatically marked as read-only after the first usage.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jul 30, 2024

IIRC, in STJ, the JsonSerializerOptions are automatically marked as read-only after the first usage.

I've looked at it and it does seem to be the case that it's not a matter of declaring it readonly in oc, perhaps we could open up an Action delegate to allow for additional tweaking logic to be added before the program is initialized

@MikeAlhayek
Copy link
Member

Why do you want to add more converters to the default options that OC uses? You can create your own Default options and use that instead if needed.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jul 30, 2024

There may be many times when it is necessary to use existing code in OC for serialisation

image

@sebastienros
Copy link
Member

Can you provide a scenario where the Default options are used by Orchard and you need another converter than the existing ones? That would either show that OC is missing a converted, or that it should not use Default there.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Aug 2, 2024

For example, this issue: #16288
If we apply OC 2.0 to our production environment, we still need to wait for the official OC merge.
Our customers might go crazy 😱.

Of course, for issue #16288 , we can fix the error in it by adjusting the script in the workflow to achieve that
But if there are more references. Misreferences in scripts can be difficult to find

@sebastienros
Copy link
Member

@hyzx86 can you file an issue corresponding to #16288 so we can understand the problem and triage it for 2.0? I don't understand the reason of the PR to throw an exception.

Copy link

It seems that this issue didn't really move for quite a while despite us asking the author for further feedback. Is this something you'd like to revisit any time soon or should we close? Please reply.

@github-actions github-actions bot added the stale label Aug 17, 2024
@hyzx86
Copy link
Contributor Author

hyzx86 commented Aug 18, 2024

Close it for now and reintroduce it the next time we have a problem

@hyzx86 hyzx86 closed this as completed Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants