Implement more conversions from JsonDynamicValue

[gvkries]

Reintroduces conversions to fundamental runtime types that were previously supported by Newtonsoft.Json but became unavailable following the upgrade to System.Text.Json. By incorporating these conversions, we restore the ability to handle dynamic types effectively, maintaining a seamless transition and compatibility across different JSON processing scenarios.

Fixes #15762

[coderabbitai]

Walkthrough

The updates focus on enhancing the JSON dynamic handling and content management in OrchardCore. Key improvements include streamlined property accessors in JSON dynamic classes, implementation of the IConvertible interface for better type conversions, and the introduction of new testing methods for robust validation of data types. Additionally, the ContentElement class now supports more efficient data handling with modified property setters.

Changes

File Path	Change Summary
.../Json/Dynamic/JsonDynamicArray.cs	Simplified property accessors, enhanced value setting and type handling.
.../Json/Dynamic/JsonDynamicObject.cs	Simplified property accessors, modified SetValue method signature.
.../Json/Dynamic/JsonDynamicValue.cs	Extended to implement IConvertible, added conversion operators, and type conversion methods.
.../ContentManagement.Abstractions/ContentElement.cs	Added _data field, updated Data property to sync with _dynamicObject and _data.
.../Tests/Data/ContentItemTests.cs	Renamed test method, added tests for DateTime and Text fields, introduced MyDateTimeField and MyTextField.
.../Tests/Data/JsonDynamicTests.cs	New tests for converting various data types to dynamic JSON values.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

Out of diff range and nitpick comments (1)

src/OrchardCore/OrchardCore.ContentManagement.Abstractions/ContentElement.cs (1)

16-16: Add XML documentation for the _data field to maintain consistency with other private fields and enhance code readability.

[coderabbitai]

Actionable comments posted: 3

Out of diff range and nitpick comments (1)

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicValue.cs (1)

237-252: The method BindConvert in JsonDynamicMetaObject uses reflection to find the appropriate conversion method. Ensure this does not impact performance negatively, especially in high-load scenarios.

[MikeAlhayek]

This LGTM.

@sebastienros can you please have a look at this one?

[coderabbitai]

Actionable comments posted: 3

[gvkries]

I fixed an regression of this PR, because overloads of ToString were missing.
Also I added the Value property back to JsonDynamicValue in case some one has used something like contentItem.Content.MyPart.myField.Text.Value instead of just contentItem.Content.MyPart.myField.Text. That was possible with Newtonsoft Json as well.

[MikeAlhayek]

@gvkries can you please address @sebastienros and I'll merge this PR.

[MikeAlhayek]

@sebastienros you good with the latest changes in this PR? Can we merge it?

[sebastienros]

Shouldn't this expression be cached instead since it varies only by type?

If so you can move the cache initialization code in a static constructor or it might involve unreadable code.

[MikeAlhayek]

@gvkries are you able to address the last comment so we can merge this?

[gvkries]

These expressions are wrapping another expression from the instance property Expression. We cannot cache them here.

[sebastienros]

@MikeAlhayek saw that the cached methods were never used while debugging and fixed the issue. I get the point with the Expression. Also reformatted the methods with bodies because I could not read it while working on it locally (to the point that I was confusing the ctor with a property accessor).

[sebastienros]

@gvkries what is the point of JsonDynamicValue.Value, it's only used in a test. I removed it but want to be sure it's not used "dynamically" for another reason.

[gvkries]

@gvkries what is the point of JsonDynamicValue.Value, it's only used in a test. I removed it but want to be sure it's not used "dynamically" for another reason.

I found a regression because formattable ToString() methods were used somewhere in a view. It's hard to find those references because of dynamic typing. Thats why I added Valueback too, because it may also be used by someone in a template only.
With Newtonsoft Json either contentItem.Content.MyPart.myField.Text.Value or contentItem.Content.MyPart.myField.Text was possible as well.

Therefore I think we should keep it.

[gvkries]

@MikeAlhayek saw that the cached methods were never used while debugging and fixed the issue. I get the point with the Expression. Also reformatted the methods with bodies because I could not read it while working on it locally (to the point that I was confusing the ctor with a property accessor).

Yep sorry. I introduced that bug in the last commit I did 😒

Can we please have a decision when to use expressions vs bodies? It's a total mess and everyone reviewing PRs here has a different opinion...

[MikeAlhayek]

Also reformatted the methods with bodies because I could not read it while working on it locally

@sebastienros typically we use expression-bodies method when there is a single line of code. In this case, these are would be expression-bodied methods.

[sebastienros]

a single line of code

My feedback:

From this experience I believe that it shouldn't be the complete rule. I'd prefer something like "if the whole thing, method/property declaration "plus" implementation fits in a single line of code (< N char)."

If the goal is readability, then I swear it's doing the opposite in these cases. It's still going on a new line, but now you see a bunch of line where before you have braces to delimit the declaration from the body. And I had issues determining it was a method, or even in the case I mentioned a constructor.

[MikeAlhayek]

If the goal is readability, then I swear it's doing the opposite in these cases.

No strong opinion here beside that seems to be the rule we follow. Maybe we should suggest against the use of bodied expressions for methods? This way we can follow a simple rule across the board.

[gvkries]

a single line of code

My feedback:

From this experience I believe that it shouldn't be the complete rule. I'd prefer something like "if the whole thing, method/property declaration "plus" implementation fits in a single line of code (< N char)."

If the goal is readability, then I swear it's doing the opposite in these cases. It's still going on a new line, but now you see a bunch of line where before you have braces to delimit the declaration from the body. And I had issues determining it was a method, or even in the case I mentioned a constructor.

I second that and if you don't mind, let me add one additional exception: Keep files consistent. E.g. when adding new methods either stick to the existing style or at least change the whole file (I would rather keep the existing format when making functional changes).

[gvkries]

@sebastienros Just to make sure, you still don't want the Value property?

[sebastienros]

With Newtonsoft Json either contentItem.Content.MyPart.myField.Text.Value or contentItem.Content.MyPart.myField.Text was possible as well.

So the dynamic wrapper in Newtonsoft has a similar Value property? But we don't have tests for it, and we don't document it?

Honestly a text field has a string Text property and contentItem.Content.MyPart.myField.Text would just access it and work fine. If someone found this property (it's a documented one) I don't see why they would also type .Value after.

It might not hurt but once we start adding known/documented things it's hard to remove it so that's why I often try to push hard on not adding seemingly harmless things. I didn't understand the case that would not work without .Value, could you provide another example?

[gvkries]

Yes, Newtonsoft has this Value property. I added that here just for backward compatibility, beside that I see no further need or justification for it. I only wanted to be sure it's not an oversight during merging.

[sebastienros]

I checked the code base for .Value in a cshtml (where I assume it would be used) and only found the properties named Value in current fields, so I think it's ok to keep it this way. Thanks for the detailed investigation.