-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add JsonApiCondition #550
Add JsonApiCondition #550
Changes from all commits
d9cf435
00c73ac
628837b
fd702b3
a359bc3
9b605d8
bd57d9f
30ab3c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import { z } from 'zod'; | ||
|
||
import { Condition } from '../condition'; | ||
import { | ||
OmitConditionType, | ||
returnValueTestSchema, | ||
} from '../shared'; | ||
|
||
export const JsonApiConditionType = 'json-api'; | ||
|
||
export const JsonApiConditionSchema = z.object({ | ||
conditionType: z.literal(JsonApiConditionType).default(JsonApiConditionType), | ||
endpoint: z.string().url(), | ||
parameters: z.record(z.string(), z.unknown()).optional(), | ||
query: z.string().optional(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how to implement this, but this field is expected to be a valid JSONPath query. It may be helpful to consumers to have a taco-web level validation of these queries to prevent invalid conditions from being created. Perhaps this can be done with a jsonpath library or a rudimentary regex. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as i mentioned on the call, any string is a valid json path. The path only becomes valid/invalid when there is some json associated with it, which will only happen at decryption time also here https://github.com/nucypher/sprints/issues/23#issuecomment-2238759313 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Json path strings have a specific format/syntax. It would be good to ensure that the format/syntax is correct at construction time. Perhaps https://www.npmjs.com/package/jsonpath / https://www.npmjs.com/package/@types/jsonpath can be of use for validation. Here's an example we did for SIWE messages - https://github.com/nucypher/taco-web/pull/527/files#diff-979bf898c06acdb90eb18e31377b0d4423990d3299e14a7596b3efca73272b49R20. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, perhaps I missed that comment, my bad.
This is a good point for syntactically valid expressions, however...
This is not true since it's possible to create syntactically invalid json path expressions. Nonetheless, this is not a blocking issue but does represent a small improvement in developer experience. Here are some reasonable (imo) examples:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is incorrect, without any json to compare with I have gone through this with some of the json path libraries already
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, i'll add it back in and some tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's a watered-down example of how nodes will attempt to process this query, and fail due to syntactic error. In [56]: import jsonpath_ng
In [57]: jsonpath_ng.parse('$.store.book[?(@.price < ]')
...
JsonPathLexerError: Error on line 1, col 13: Unexpected character: ? corrected expression with enclosing quotes and escapes: In [58]: jsonpath_ng.parse('$.store[\'book[?(@.price < ]\']')
Out[58]: Child(Child(Root(), Fields('store')), Fields('book[?(@.price < ]')) Another example: In [61]: jsonpath_ng.parse('$.store.book[*')
...
JsonPathParserError: Parse error near the end of string! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah i understand this - my point is that if the field There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe when I had this in my testing examples where wrong, but I've added it back in 4546b2f There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's only syntactically valid if the expression has the correct enclosing quotes ( My point is: it is possible to create syntactically invalid jsonpath expressions. There's nothing we can do to validate that the expression will be valid against a JSON endpoint's response structure but we can easily detect syntax errors. |
||
returnValueTest: returnValueTestSchema, // Update to allow multiple return values after expanding supported methods | ||
}); | ||
|
||
export type JsonApiConditionProps = z.infer<typeof JsonApiConditionSchema>; | ||
|
||
export class JsonApiCondition extends Condition { | ||
constructor(value: OmitConditionType<JsonApiConditionProps>) { | ||
super(JsonApiConditionSchema, { | ||
conditionType: JsonApiConditionType, | ||
...value, | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
/* eslint-disable @typescript-eslint/no-unused-vars */ | ||
import { describe, expect, it } from 'vitest'; | ||
|
||
import { | ||
JsonApiCondition, | ||
JsonApiConditionSchema, | ||
} from '../../../src/conditions/base/json-api'; | ||
import { testJsonApiConditionObj } from '../../test-utils'; | ||
|
||
describe('JsonApiCondition', () => { | ||
describe('validation', () => { | ||
it('accepts a valid schema', () => { | ||
const result = JsonApiCondition.validate( | ||
JsonApiConditionSchema, | ||
testJsonApiConditionObj, | ||
); | ||
|
||
expect(result.error).toBeUndefined(); | ||
expect(result.data).toEqual(testJsonApiConditionObj); | ||
}); | ||
|
||
it('rejects an invalid schema', () => { | ||
const badJsonApiObj = { | ||
...testJsonApiConditionObj, | ||
endpoint: 'not-a-url', | ||
}; | ||
|
||
const result = JsonApiCondition.validate(JsonApiConditionSchema, badJsonApiObj); | ||
|
||
expect(result.error).toBeDefined(); | ||
expect(result.data).toBeUndefined(); | ||
expect(result.error?.format()).toMatchObject({ | ||
endpoint: { | ||
_errors: ['Invalid url'], | ||
}, | ||
}); | ||
}); | ||
|
||
describe('parameters', () => { | ||
it('accepts conditions without query path', () => { | ||
const { query, ...noQueryObj} = testJsonApiConditionObj; | ||
const result = JsonApiCondition.validate( | ||
JsonApiConditionSchema, | ||
noQueryObj | ||
); | ||
|
||
expect(result.error).toBeUndefined(); | ||
expect(result.data).toEqual(noQueryObj); | ||
}); | ||
|
||
it('accepts conditions without parameters', () => { | ||
const { query, ...noParamsObj} = testJsonApiConditionObj; | ||
const result = JsonApiCondition.validate( | ||
JsonApiConditionSchema, | ||
noParamsObj | ||
); | ||
|
||
expect(result.error).toBeUndefined(); | ||
expect(result.data).toEqual(noParamsObj); | ||
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
parameters
take any primitive type as a parameter, or juststring
andobject
? Looking at the tests etc., I'm under the impression it'sobject
only.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. The example lingo from the issue is:
so i guess it could be
z.record(z.string(), z.object()).optional(),