fix: a more complete implementation of schema generation#1550
Conversation
Summary of ChangesHello @kevmoo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the JSON schema generation feature in 'json_serializable'. The primary goal was to ensure that the generated schemas accurately reflect the 'required' status and default values of fields in Dart classes, especially when dealing with constructor parameters and various field configurations. This leads to more precise and reliable schema outputs, improving interoperability and validation for generated JSON. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
- parse out the default value from ctor param default - exclude getter-only properties - properly exclude properties not set via constructor from required set - correctly include required nullable ctor params in required set Fixes #1549
bb2dcd0 to
1614242
Compare
There was a problem hiding this comment.
Code Review
This pull request provides a more complete and correct implementation for JSON schema generation. The changes in schema_helper.dart significantly improve the logic for determining which fields to include in the schema and whether they are required, considering constructor parameters, default values, and initializers. The new and updated test cases, especially schema_example.dart, provide good coverage for these improvements. My feedback is minor and related to a few typos in the new test code.
this is going to break things. Then we'll fix Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@Mithos5r - please take another look here Turns out AI-generated code really needs to be reviewed carefully! |
|
Hi! I saw your comment on the issue and I've replied there. The issue should be closed with the current commit (fe5f161), but I noticed that $defs loses the reference: 'branch': {r'$ref': r'#/$defs/List'}. Since this is currently broken, perhaps it could be fixed within this PR? JSON Schema is becoming the standard for cross-platform communication. It’s not just for server-side validation; it's also essential for Model Context Protocol (MCP) and other LLM tool-calling interfaces. If the schema is broken, the AI cannot correctly understand the expected input structure. I think that with that fix, the feature will be working well enough to be solid. |
|
@Mithos5r – I think I got it here. Please review! |
|
I cannot test the commit: Because every version of json_serializable from git depends on json_annotation >=4.11.0 <4.12.0 and borrame depends on json_annotation 4.10.0, json_serializable from git is forbidden. And if i override the depencency i can get the packages but i cannot exe run build_runner: pubspec_overrides.yaml dependency_overrides:
json_annotation: 4.10.0dart run build_runner build --delete-conflicting-outputs Built build_runner:build_runner.
2s compiling builders/jit
build_runner
E ../../../.pub-cache/git/json_serializable-ed225bb61681fd2e5a34beb422cc81a9d8608134/json_serializable/lib/src/type_helpers/config_types.dart:113:29: Error: The getter 'dateTimeUtc'
isn't defined for the type 'JsonSerializable'.
- 'JsonSerializable' is from 'package:json_annotation/src/json_serializable.dart' ('../../../.pub-cache/hosted/pub.dev/json_annotation-4.10.0/lib/src/json_serializable.dart').
Try correcting the name to the name of an existing getter, or defining a getter or field named 'dateTimeUtc'.
dateTimeUtc: config.dateTimeUtc ?? ClassConfig.defaults.dateTimeUtc,
^^^^^^^^^^^
../../../.pub-cache/git/json_serializable-ed225bb61681fd2e5a34beb422cc81a9d8608134/json_serializable/lib/src/type_helpers/config_types.dart:166:5: Error: No named parameter with the
name 'dateTimeUtc'.
dateTimeUtc: dateTimeUtc,
^^^^^^^^^^^
../../../.pub-cache/hosted/pub.dev/json_annotation-4.10.0/lib/src/json_serializable.dart:268:9: Context: Found this candidate, but the arguments don't match.
const JsonSerializable({
^^^^^^^^^^^^^^^^
../../../.pub-cache/git/json_serializable-ed225bb61681fd2e5a34beb422cc81a9d8608134/json_serializable/lib/src/utils.dart:69:3: Error: No named parameter with the name 'dateTimeUtc'.
dateTimeUtc: reader.read('dateTimeUtc').literalValue as bool?,
^^^^^^^^^^^
../../../.pub-cache/hosted/pub.dev/json_annotation-4.10.0/lib/src/json_serializable.dart:268:9: Context: Found this candidate, but the arguments don't match.
const JsonSerializable({
^^^^^^^^^^^^^^^^
../../../.pub-cache/git/json_serializable-ed225bb61681fd2e5a34beb422cc81a9d8608134/json_serializable/lib/src/utils.dart:133:29: Error: The getter 'dateTimeUtc' isn't defined for the
type 'JsonSerializable'.
- 'JsonSerializable' is from 'package:json_annotation/src/json_serializable.dart' ('../../../.pub-cache/hosted/pub.dev/json_annotation-4.10.0/lib/src/json_serializable.dart').
Try correcting the name to the name of an existing getter, or defining a getter or field named 'dateTimeUtc'.
dateTimeUtc: annotation.dateTimeUtc ?? config.dateTimeUtc,
^^^^^^^^^^^
Log overflowed the console, switching to line-by-line logging.
E Failed to compile build script. Check builder definitions and generated script .dart_tool/build/entrypoint/build.dart. |
|
Try dependency_overrides:
json_serializable:
git:
url: https://github.com/google/json_serializable.dart.git
ref: fix_schema_silly
path: json_serializable
json_annotation:
git:
url: https://github.com/google/json_serializable.dart.git
ref: fix_schema_silly
path: json_annotation |
|
Yes, that's right! I never thought about changing both, I was stubborn about changing only json_serializable. I’ve reviewed the latest generated schema and I am pretty sure that i it's correct. This PR opens up so many possibilities, automated documentation, and robust server-side validation and MCP. As more users start adopting it for their specific workflows, we will likely discover new corner cases or specific requirements. Thank you so much for your hard work and for the quick iterations. From my side, this is ready to be merged! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the JSON schema generation feature by refactoring the logic into schema_helper.dart and addressing several bugs. The changes correctly handle default values from constructor parameters, exclude getter-only properties, and more accurately determine required fields. The addition of comprehensive integration tests is a great step towards ensuring the robustness of the new logic. The code is well-structured, and the changes are clear. I have a couple of minor suggestions for code cleanup.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request provides a more complete and correct implementation for JSON schema generation. It addresses several issues, including handling of default values from constructor parameters, exclusion of getter-only properties, and more accurate determination of required properties. The changes involve a significant refactoring, moving the schema generation logic into schema_helper.dart and adding comprehensive tests to validate the new behavior.
Overall, the changes are excellent and greatly improve the schema generation feature. I have one suggestion to simplify a piece of logic in schema_helper.dart to improve readability and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the JSON schema generation by providing a more complete implementation that handles various edge cases like default values from constructors, getter-only properties, and required nullable parameters. The refactoring to consolidate schema generation logic into schema_helper.dart is a good architectural change. The added tests are comprehensive and provide good coverage for the new logic. I've found one issue with how documentation comments are parsed, which could lead to corrupted descriptions in the generated schema.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request provides a much more complete and correct implementation of JSON schema generation. It addresses several issues, including handling of default values from constructor parameters, exclusion of getter-only properties, and more accurate determination of required properties. The refactoring moves the schema generation logic into schema_helper.dart and adds comprehensive test cases, including integration tests with the json_schema package. The changes are a significant improvement. I have one suggestion to make the logic for finding constructor parameters even more robust.
Fixes #1549