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

Json codable no type assumption in custom fromJson #59581

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

Conversation

cedvdb
Copy link
Contributor

@cedvdb cedvdb commented Nov 21, 2024

json_codable wrongly assumes fromJson receives a Map

This allows an user defined class to have a fromJson method with a parameter that is not a Map. Before this change the @JsonCodable macro would check if a field has a fromJson constructor and assume that the parameter type was a Map. This PR uses the type of the parameter instead.

Note: I have this weird behavior where I can only run the tests with the macros enabled once per branch, so I have to switch branches and run the tests on another.

@cedvdb cedvdb changed the title Json codable no map assumption Json codable no type assumption in custom fromJson Nov 21, 2024
Copy link

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/396629

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

3 similar comments
Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

@cedvdb cedvdb force-pushed the json_codable_no_map_assumption branch from cc49d86 to ba39aa4 Compare November 21, 2024 11:32
Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

3 similar comments
Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

@cedvdb cedvdb changed the title Json codable no type assumption in custom fromJson [draft] Json codable no type assumption in custom fromJson Nov 22, 2024
Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

@cedvdb cedvdb changed the title [draft] Json codable no type assumption in custom fromJson Json codable no type assumption in custom fromJson Nov 22, 2024
Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

@jakemac53 jakemac53 self-assigned this Nov 22, 2024
Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/396629 has been updated with the latest commits from this pull request.

@cedvdb
Copy link
Contributor Author

cedvdb commented Nov 22, 2024

It seems there is already a PR for this here : #56665

@jakemac53
Copy link
Contributor

It seems there is already a PR for this here : #56665

That one I think is abandoned

@natebosch
Copy link
Member

Can a class also define how to handle null? Would factory Foo.fromJson(Object? o) or factory Foo.fromJson(String? s) work? Can we add a test if so?

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.

[json_codable] the object passed to fromJson is not necessarily a Map
3 participants