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

Quick Schema fixes #201

Merged
merged 5 commits into from
Apr 19, 2024
Merged

Quick Schema fixes #201

merged 5 commits into from
Apr 19, 2024

Conversation

eyadmba
Copy link
Contributor

@eyadmba eyadmba commented Apr 17, 2024

No description provided.

@eyadmba eyadmba requested a review from a team as a code owner April 17, 2024 22:23
Comment on lines +33 to 40
},
"lastSeenOn": {
"description": "The timestamp (in milliseconds since epoch) when the device either last checked in or was scanned.",
"type": ["number", "null"],
"format": "date-time"
}
},
"lastSeenOn": {
"description": "The timestamp (in milliseconds since epoch) when the device either last checked in or was scanned.",
"type": ["number", "null"],
"format": "date-time"
},
"required": ["function", "lastSeenOn"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was living outside the properties key...

@@ -9,8 +9,7 @@
},
{
"properties": {},
"required": [],
"excludes": []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

excludes is causing issues with AJV, and while this is not a solution, but it will minimize the number of errors that we're getting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll have to figure out a proper way to support this functionality eventually though.

@@ -1,6 +1,6 @@
{
"name": "@jupiterone/data-model",
"version": "0.56.1",
"version": "0.57.0",
Copy link
Contributor Author

@eyadmba eyadmba Apr 17, 2024

Choose a reason for hiding this comment

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

Alright so, this is weird. I don't know if this is considered a breaking change or not, but I don't think it is.

Moving the HostAgent's lastSeenOn inside the properties key is technically adding a new field that didn't exist before.
But also, the schema was mentioning the lastSeenOn inside the required properties list...

To be able to tell if it's a breaking change or not, we'd have to understand how existing consumers of the HostAgent schema handle a required property that isn't actually defined in the schema's properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

const Ajv = require('ajv');

const ajv = new Ajv({
  // extendRefs: true 
});

ajv.addSchema({
  "title": "test",
  "type": "object",
  "properties": {
    "abc": {
      "type": "string"
    }
  },
  "required": ["test"]
}
, 'a.json');

console.log(ajv.validate('a.json', {}));

console.log(ajv.validate('a.json', { test: 1 }));

console.log(ajv.validate('a.json', { test: true }));

console.log(ajv.validate('a.json', { test: 'string' }));
false
true
true
true

Looks like AJV considers this as requiring the property to be present but no restriction on the type.

@eyadmba eyadmba merged commit 790c496 into main Apr 19, 2024
12 checks passed
@eyadmba eyadmba deleted the users/eyad/fix-schema branch April 19, 2024 18:07
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