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 schema parsing strips extra properties #5275

Merged

Conversation

shauns
Copy link
Contributor

@shauns shauns commented Jan 27, 2025

JSON schema validation is tweaked to strip additional properties that aren't parsed by the schema. This lets us use JSON schema to parse partial config objects, which is what we need for parsing app.toml from JSON schema only (otherwise, validation either has to fail, or we don't know what slice of the config was relevant to the schema)

I've preserved the existing behaviour as much as possible, and this is triggered only when UID strategy === single.

Copy link
Contributor Author

shauns commented Jan 27, 2025

Copy link
Contributor

github-actions bot commented Jan 27, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.42% (+0.03% 🔼)
8967/11890
🟡 Branches
70.54% (+0.02% 🔼)
4358/6178
🟡 Functions
75.22% (+0.04% 🔼)
2352/3127
🟡 Lines
75.93% (+0.04% 🔼)
8472/11157
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% (-2.7% 🔻)
90.48% 98.61%
🟢
... / fetch-extension-specifications.ts
93.88% (-3.74% 🔻)
80.95% (-2.38% 🔻)
100%
95.24% (-4.76% 🔻)
🔴
... / json-schema.ts
28%
21.05% (-1.17% 🔻)
33.33% 28%

Test suite run success

2015 tests passing in 905 suites.

Report generated by 🧪jest coverage report action from 4007c75

@shauns shauns added the #gsd:37045 label Jan 27, 2025 — with Graphite App
@shauns shauns marked this pull request as ready for review January 27, 2025 11:52
@shauns shauns requested a review from a team as a code owner January 27, 2025 11:52
Copy link
Contributor

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

@shauns shauns force-pushed the shauns/01-27-json_schema_parsing_strips_extra_properties branch from ef6c4b7 to ae8f1b5 Compare January 27, 2025 12:12
Base automatically changed from shauns/01-27-remotespecfication_includes_uidisclientprovided to main January 27, 2025 12:27
@shauns shauns force-pushed the shauns/01-27-json_schema_parsing_strips_extra_properties branch 2 times, most recently from 13991f2 to 61faa92 Compare January 27, 2025 12:33
@shauns shauns self-assigned this Jan 27, 2025
Copy link
Contributor

@isaacroldan isaacroldan left a comment

Choose a reason for hiding this comment

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

Just a small comment, but not blocking

@shauns shauns force-pushed the shauns/01-27-json_schema_parsing_strips_extra_properties branch from 61faa92 to d92ad26 Compare January 28, 2025 12:33

Unverified

This user has not yet uploaded their public signing key.
@shauns shauns force-pushed the shauns/01-27-json_schema_parsing_strips_extra_properties branch from d92ad26 to 4007c75 Compare January 28, 2025 12:41
Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/json-schema.d.ts
@@ -1,5 +1,6 @@
 import { ParseConfigurationResult } from './schema.js';
 import { ErrorObject, SchemaObject } from 'ajv';
+export type HandleInvalidAdditionalProperties = 'strip' | 'fail';
 type AjvError = ErrorObject<string, {
     [key: string]: unknown;
 }>;
@@ -19,10 +20,11 @@ export declare function normaliseJsonSchema(schema: string): Promise<SchemaObjec
  *
  * @param subject - The object to validate.
  * @param schema - The JSON schema to validate against.
+ * @param handleInvalidAdditionalProperties - Whether to strip or fail on invalid additional properties.
  * @param identifier - The identifier of the schema being validated, used to cache the validator.
  * @returns The result of the validation. If the state is 'error', the errors will be in a zod-like format.
  */
-export declare function jsonSchemaValidate(subject: object, schema: SchemaObject, identifier: string): ParseConfigurationResult<unknown> & {
+export declare function jsonSchemaValidate(subject: object, schema: SchemaObject, handleInvalidAdditionalProperties: HandleInvalidAdditionalProperties, identifier?: string): ParseConfigurationResult<unknown> & {
     rawErrors?: AjvError[];
 };
 export {};
\ No newline at end of file

@shauns shauns added this pull request to the merge queue Jan 28, 2025
Merged via the queue into main with commit 517f04e Jan 28, 2025
28 checks passed
@shauns shauns deleted the shauns/01-27-json_schema_parsing_strips_extra_properties branch January 28, 2025 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants