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

feat: internal to Standard JSON Schema conversion COMPASS-8700 #219

Merged

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Jan 31, 2025

Very similar to $jsonSchema. The difference is that instead of mapping the internal types to bsonType, we map it to JSON Schema.
This is done so that it fits the Relaxed Extended JSON definition - it will be either

  • one of the basic JSON Schema types
  • refer to a definition describing the relaxed extended JSON

The definition are mostly taken from @mcasimir's POC, I checked them with specs and made several updates:

  • binary subtype pattern changed from ^[0-9a-fA-F]{2}$' to `^[0-9a-fA-F]{1,2}$' (it says "BSON binary type as a one- or two-character hex string" in the specs)
  • removed maximum on timestamp t&i (I just don't see it in the specs)
  • 'Infinity', '-Infinity', 'NaN' are not direct values, but wrapped in{ $numberDouble: ... }

TODO:

@paula-stacho paula-stacho marked this pull request as ready for review February 3, 2025 15:47
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

Couple small things, looks good, looking at the tests now

return { required, properties };
}

export default async function internalSchemaToStandard(
Copy link
Contributor

@mcasimir mcasimir Feb 3, 2025

Choose a reason for hiding this comment

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

silly NIT comment, (but is your fault for tagging me in the PR description :)), from an external PoV, even having read the scope, it took me a while to link "standard" here to the distinction between the non-standard mongodb-specific json schema, and the standard json schema. I wonder if after some time will pass from this project this context would be even more hard to get.

How about we add a comment to explain this distinction, or maybe a more boring name that carries the context within itself something like toStandardEJSONJsonSchema vs toMongodbJsonSchema? Totally non blocking, please feel free to ignore!

Copy link
Contributor Author

@paula-stacho paula-stacho Feb 4, 2025

Choose a reason for hiding this comment

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

Not sure about the naming, but I'm adding JSDoc descriptions to the top level functions (in the accessor)

type: 'object',
required,
properties,
$defs: RELAXED_EJSON_DEFINITIONS
Copy link
Contributor

@mcasimir mcasimir Feb 3, 2025

Choose a reason for hiding this comment

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

non-blocking NIT: do we want to include all of the definitions, even for types that aren't used? Omitting some would make the schema easier to read, not sure if is worth to do that though.

$defs: RELAXED_EJSON_DEFINITIONS,
properties: {
mixedType: {
type: ['integer', 'string']
Copy link
Contributor

@mcasimir mcasimir Feb 3, 2025

Choose a reason for hiding this comment

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

is this valid json schema? i thought here you need

"anyOf": [
    { "type": "integer" },
    { "type": "string" }
  ]

please ignore if that compact form is also possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compact form is possible as long as it's plain types (no extra properties or refs)

Copy link
Contributor

@mcasimir mcasimir left a comment

Choose a reason for hiding this comment

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

Thanks @paula-stacho! I left few random comments.

Would it also make sense to load ajv in our tests and validate the schema that we produce against the json schema meta-schema? To make sure that we are effectively producing a valid json schema, and that we would keep doing so even after changes to the code?

@paula-stacho
Copy link
Contributor Author

Thanks @paula-stacho! I left few random comments.

Would it also make sense to load ajv in our tests and validate the schema that we produce against the json schema meta-schema? To make sure that we are effectively producing a valid json schema, and that we would keep doing so even after changes to the code?

Good idea, adding this validation!

Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

Looking good, I left a couple comments on types. A few questions to help my understanding as well

Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

nice!

@paula-stacho paula-stacho merged commit c514068 into COMPASS-6862-schema-export-multiple-formats Feb 5, 2025
18 checks passed
@paula-stacho paula-stacho deleted the COMPASS-8700 branch February 5, 2025 14:06
paula-stacho added a commit that referenced this pull request Feb 6, 2025
…S-8702 COMPASS-8709 (#222)

* feat: add analyzeDocuments + SchemaAccessor COMPASS-8799 (#216)



---------

Co-authored-by: Anna Henningsen <[email protected]>

* feat: internal to MongoDB $jsonSchema conversion COMPASS-8701 (#218)


---------

Co-authored-by: Anna Henningsen <[email protected]>

* feat: internal to Standard JSON Schema conversion COMPASS-8700 (#219)

* feat: internal to Expanded JSON Schema conversion COMPASS-8702  (#220)

---------

Co-authored-by: Anna Henningsen <[email protected]>
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.

3 participants