-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[For Discussion] Move JsonData to Core #31769
Closed
Closed
Changes from 28 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
8b410e8
Move JsonData to Core
annelo-msft e3113b4
update namespace to have JsonData side-by-side with JsonPatchDocument
annelo-msft 40d53ed
Merge remote-tracking branch 'upstream/main' into annelo/core-jsondata
annelo-msft 770fea4
move perf tests
annelo-msft 5a0a54d
export APIs
annelo-msft 7d308cb
Merge remote-tracking branch 'upstream/main' into annelo/core-jsondata
annelo-msft 607d3f4
add some tests
annelo-msft ba8ce15
make castable from arbitrary type
annelo-msft 73ba1f6
add DateTime cast test
annelo-msft f736db3
make internal members private and implement BindGetIndex
annelo-msft 0f2f374
make jsonData internal; test introduces an issue with int equality th…
annelo-msft d0b4e07
tidyup
annelo-msft abe7c77
Merge remote-tracking branch 'upstream/main' into annelo/core-jsondata
annelo-msft bf207c7
refactoring and calling cast operators dynamically
annelo-msft 8ebad75
add test project to test only public API; implement BindBinaryOperation
annelo-msft 52c5123
add more public tests; string equality still fails when dynamic objec…
annelo-msft 3958786
Merge remote-tracking branch 'upstream/main' into annelo/core-jsondata
annelo-msft 562b773
move 'special array property' logic into BindGetMember
annelo-msft 0856479
make JsonData public; update CLU sample with JsonData
annelo-msft 5167a1f
ignore case when looking up property; add ToString() for number type.
annelo-msft d2b7d71
return null for optional properties
annelo-msft 7e5408d
continuing to port CLI samples.
annelo-msft e19af63
move samples to JsonData
annelo-msft 64ef81a
update snippets
annelo-msft 9496dbe
Merge remote-tracking branch 'upstream/main' into annelo/core-jsondata
annelo-msft 146bcc8
illustrate more samples with JsonData
annelo-msft 0aca600
bug fix
annelo-msft d84bdd3
add tests for cases found during samples update
annelo-msft 8118f74
Merge remote-tracking branch 'upstream/main' into annelo/core-jsondata
annelo-msft 795e2ff
Dispose JsonDocument
annelo-msft a0fadb7
Merge remote-tracking branch 'upstream/main' into annelo/core-jsondata
annelo-msft File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @KrzysztofCwalina offline, but I don't think we should automatically convert camelCase JSON properties to PascalCase .NET properties on responses. Requests don't get the same treatment, so this may be confusing or even misleading. Say you get a response back, want to update it, and then send it back (which you also can't since JsonData is effectively immutable):
I appreciate we want to be idiomatic, but as soon as customers are having to deal with raw JSON - via System.Text.Json like my samples were before vs. what you're prototyping here with
JsonData
(which is a remarkable improvement) - I don't think we should "auto-correct" anything. Python nor JS would since customers are working directly with the JSON data. I don't think C# / .NET should either.Model classes give us this ability, and once we generate them (for Cadl or otherwise) we can and should do that translation because we can do so consistently.
/cc @tg-msft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I don't understand the thinking there. We do this in our public models, and the idea is that JsonData is a stand-in until we add those types. Ideally, the customer would have to change very little to move to any types we added to a library. It's not that they're dealing with raw Json (which they can do easily if they want) -- it's a layer over the Json (or Xml, or whatever) that makes it conform to a higher .NET standard for look-and-feel. Is consistency with anonymous types your primary concern here, because I agree that is not good. Are there any other considerations I'm not thinking of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see that this is a question about round-tripping the type, my apologies for having missed that the first time. We are planning to provide a separate API to handle-round tripping, that may be able to address this. I am still curious to know if there are other considerations here beyond the inconsistency across input/output that we should also be aware of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume with my example above that the JSON schema is indeed camelCase, as most services define (and should): the request would accept
id
andfoo
, notId
andFoo
. However, by translating from camelCase to PascalCase we could give callers the impression that the properties are indeedId
andFoo
.This isn't just about roundtripping, though. It's about perception. If all the response properties are PascalCase, why wouldn't many think that request properties are too?
I appreciate this isn't "raw" JSON. That's not what I intended. But it's a very thin wrapper over raw JSON that is meant to provide a JSON-like interaction for the customer, yes? By morphing into something it's not just to seem idiomatic, we create this confusion of casing for customers. Why not just leave it with the exact shape - casing and all - as the service returns (response) and expects (request)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's totally fair - all good points. It feels like the question hinges on how thin the wrapper is and how well we document and illustrate via samples what the type is actually doing. If, for example, it also wraps other formats like XML, it might be more clear that you can't make too many assumptions about what's underneath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this supports XML, I don't see my point changing: if an XML document uses camelCase or PascalCase (and in my experience it varies even more than with JSON, which is more often camelCase), we should leave casing as is to avoid confusion. If we start showing that customers can always use PascalCase for responses, why wouldn't they think they can use it for input as well? Or if they use the
obj["foo"]["bar"]
pattern it will also vary fromobj.Foo.Bar
. Changing cases like that just seems so jarring.What benefit to the customer does translating the casing have? I can certainly see a lot of cons. The only pro is "it feels like C#", but they aren't working with generated types (C# or otherwise): they are working with thin wrappers over raw JSON, XML, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should
ToDynamic
take an optionalObjectSerailizer
so folks can customize this? I feel like that would easily allow this to work for people who wanted it, but then we could get away with less magic by default if we didn't change the case.