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.

Sign up for GitHub

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

Jump to bottom

Implement more conversions from JsonDynamicValue #15816

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Implement more conversions from JsonDynamicValue #15816

wants to merge 14 commits into from
+875 −84

Conversation

gvkries
Copy link
Contributor

@gvkries gvkries commented Apr 24, 2024

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 coderabbitai
Copy link
Contributor

coderabbitai bot commented Apr 24, 2024 •
edited

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
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[bot]
coderabbitai bot reviewed Apr 24, 2024
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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.

src/OrchardCore/OrchardCore.ContentManagement.Abstractions/ContentElement.cs Show resolved Hide resolved
coderabbitai[bot]
coderabbitai bot reviewed Apr 24, 2024
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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.

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicValue.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicValue.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicValue.cs Show resolved Hide resolved
MikeAlhayek
MikeAlhayek approved these changes Apr 24, 2024
View reviewed changes
Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

This LGTM.

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

coderabbitai[bot]
coderabbitai bot reviewed Apr 25, 2024
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicValue.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicValue.cs Show resolved Hide resolved
src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicValue.cs Outdated Show resolved Hide resolved
@gvkries
Copy link
Contributor Author

gvkries commented Apr 25, 2024

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.

This was referenced May 1, 2024
Open
Open
@MikeAlhayek
Copy link
Member

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

@sebastienros sebastienros mentioned this pull request May 8, 2024
Open
@MikeAlhayek
Copy link
Member

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

sebastienros
sebastienros reviewed May 8, 2024
View reviewed changes
src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicValue.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicValue.cs
{
_value = JsonValue?.GetObjectValue();
_hasValue = true;
var convertExpression = Expression.Convert(Expression.Convert(Expression, typeof(JsonDynamicValue)), targetType, castMethod);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@sebastienros
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor Author

gvkries commented May 13, 2024

@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
Copy link
Contributor Author

gvkries commented May 13, 2024

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastienros sebastienros sebastienros approved these changes

@hyzx86 hyzx86 hyzx86 left review comments

@coderabbitai coderabbitai[bot] coderabbitai left review comments

@MikeAlhayek MikeAlhayek Awaiting requested review from MikeAlhayek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

System.Text.Json and dynamic types
4 participants
@gvkries @MikeAlhayek @sebastienros @hyzx86