Add JsonApiCondition #550

theref · 2024-07-17T11:38:43Z

Fixes https://github.com/nucypher/sprints/issues/23

netlify · 2024-07-17T11:38:59Z

✅ Deploy Preview for taco-nft-demo ready!

Name	Link
🔨 Latest commit	`be45583`
🔍 Latest deploy log	https://app.netlify.com/sites/taco-nft-demo/deploys/66a8f21fd4779e00080c641d
😎 Deploy Preview	https://deploy-preview-550--taco-nft-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-07-17T11:38:59Z

✅ Deploy Preview for taco-demo ready!

Name	Link
🔨 Latest commit	`be45583`
🔍 Latest deploy log	https://app.netlify.com/sites/taco-demo/deploys/66a8f21f05d49c0009ffaf26
😎 Deploy Preview	https://deploy-preview-550--taco-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

derekpierre · 2024-07-19T12:22:05Z

Just a side note - I don't think this should go into main; at least not yet. The next release of taco-web (SIWE etc.) should not have this code.

JsonApiCondition needs to be released on the server-side first and won't be released until 7.5.0. Perhaps instead you merge this PR into an epic/dev branch for now? Perhaps there's an existing alternative strategy that @piotr-roslaniec already uses...?

theref · 2024-07-19T12:38:47Z

The original issue https://github.com/nucypher/sprints/issues/23 specifically says "Get taco-web preemptively compatible..." so I assumed there was some reason for having this merged before 7.5.0 is release

derekpierre · 2024-07-19T12:47:19Z

The original issue nucypher/sprints#23 specifically says "Get taco-web preemptively compatible..." so I assumed there was some reason for having this merged before 7.5.0 is release

Yep. The goal is for everyone to start getting more familiar with taco-web. This task pushes that forward.

The "pre-emptive" part is what I'm referring to. The code will be used, just not in the next release of taco-web. The next release of taco-web pretty much boils down to Sign-in With Ethereum support (#527 and some other fixes). If we merge this PR into main then it will be co-mingled with SIWE support which is not what we want. These are the steps for nucypher 7.4.0 release and subsequent taco-web release - https://github.com/nucypher/sprints/issues/5#issuecomment-2209378181.

Then a similar set of steps for the 7.5.0 nucypher release (w/ JsonApiCondition support) and subsequent taco-web release but instead of SIWE it'll be the JsonApiCondition feature.

packages/taco/test/test-utils.ts

KPrasch · 2024-07-30T11:45:15Z

packages/taco/src/conditions/base/json-api.ts

+  conditionType: z.literal(JsonApiConditionType).default(JsonApiConditionType),
+  endpoint: z.string().url(),
+  parameters: z.record(z.string(), z.unknown()).optional(),
+  query: z.string().optional(),


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.

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

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.

as i mentioned on the call

Ah, perhaps I missed that comment, my bad.

The path only becomes valid/invalid when there is some json associated with it

This is a good point for syntactically valid expressions, however...

any string is a valid json path

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:

$.store.book[?(@.price < ]
The filter expression is incomplete and not properly closed.

$[store][book]
~~Incorrect usage of square brackets. Proper syntax is $.store.book or $.store['book'].~~
This one is actually parsable but does not follow the spec, I'll let it slide :-)

$.store.book[*
Asterisk wildcard is not properly closed with a bracket.

this is incorrect, without any json to compare with $.store.book[?(@.price < ] is a valid json path, because the key in the dictionary could be the string "book[?(@.price < ]"

I have gone through this with some of the json path libraries already

$[store][book] again, the key could be the string "[store][book]"

ok, i'll add it back in and some tests

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!

yeah i understand this - my point is that if the field Fields('book[?(@.price < ]') exists then it's a valid json path

maybe when I had this in my testing examples where wrong, but I've added it back in 4546b2f
Currently it fails, due to the path being valid without any json to compare with

yeah i understand this - my point is that if the field Fields('book[?(@.price < ]') exists then it's a valid json path

It's only syntactically valid if the expression has the correct enclosing quotes ('$.store[\'book[?(@.price < ]\']'). Also consider unclosed square brackets for example ('$.store.book[*' is syntactically invalid but can be made valid by adding the appropriate quote and square bracket '$.store[\'book[*\']' is valid).

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.

KPrasch

Overall looks good 👍🏻 I'm glad to see that this was a straight-forward implementation following the changes to nucypher/nucypher. I left several comments about detecting JSONPath syntax errors but it's not a blocking concern. I opened up #559 to track it. It's reasonable to address this in a follow-up PR imo.

Also it looks like there are several CI failures. It will be good to get the green check mark before merging.

Nice work!

piotr-roslaniec · 2024-07-30T14:21:12Z

packages/taco/src/conditions/base/json-api.ts

+export const JsonApiConditionSchema = z.object({
+  conditionType: z.literal(JsonApiConditionType).default(JsonApiConditionType),
+  endpoint: z.string().url(),
+  parameters: z.record(z.string(), z.unknown()).optional(),


Does parameters take any primitive type as a parameter, or just string and object? Looking at the tests etc., I'm under the impression it's object only.

Suggested change

parameters: z.record(z.string(), z.unknown()).optional(),

parameters: z.record(z.object()).optional(),

I'm not sure I follow. The example lingo from the issue is:

"parameters": { "ids": "ethereum", "vs_currencies": "usd", },

so i guess it could be z.record(z.string(), z.object()).optional(),

Update `JsonApiConditionSchema` to match lingo

JSON path will always be able to be parsed. It only becomes invalid when combined with JSON, and this will only be known at decryption time.

theref marked this pull request as draft July 17, 2024 11:39

theref requested a review from piotr-roslaniec July 17, 2024 14:15

theref force-pushed the offchain branch from e026564 to 382870a Compare July 19, 2024 09:22

theref changed the title ~~WIP: Add basic types, props, and classes for JsonApiCondition~~ WIP: Add JsonApiCondition Jul 19, 2024

theref requested a review from KPrasch July 19, 2024 11:27

piotr-roslaniec reviewed Jul 19, 2024

View reviewed changes

packages/taco/test/test-utils.ts Outdated Show resolved Hide resolved

theref marked this pull request as ready for review July 24, 2024 11:17

theref changed the title ~~WIP: Add JsonApiCondition~~ Add JsonApiCondition Jul 24, 2024

theref force-pushed the offchain branch 4 times, most recently from 404d4e2 to b99688e Compare July 30, 2024 07:52

KPrasch reviewed Jul 30, 2024

View reviewed changes

theref force-pushed the offchain branch from b99688e to be45583 Compare July 30, 2024 12:27

KPrasch approved these changes Jul 30, 2024

View reviewed changes

theref force-pushed the offchain branch from a131bae to be45583 Compare July 30, 2024 14:01

piotr-roslaniec reviewed Jul 30, 2024

View reviewed changes

andresceballosm mentioned this pull request Jul 30, 2024

Added JSONPath syntactic validation #561

Merged

7 tasks

derekpierre changed the base branch from main to epic-offchain July 30, 2024 20:51

theref and others added 6 commits July 31, 2024 12:46

Add basic types, props, and classes for JsonApiCondition

d9cf435

Update JsonApiConditionSchema to match lingo

00c73ac

Update `JsonApiConditionSchema` to match lingo

Add basic tests for json api condition type

628837b

Fix schema for json api condition

fd702b3

JSON path will always be able to be parsed. It only becomes invalid when combined with JSON, and this will only be known at decryption time.

query and parameters in JSONConditionScheme should be optional

a359bc3

Fix import ordering

9b605d8

theref added 2 commits July 31, 2024 12:46

Update test json url and add serialization test

bd57d9f

Fix linting error

30ab3c3

theref force-pushed the offchain branch from be45583 to 30ab3c3 Compare July 31, 2024 10:46

KPrasch merged commit f7c1514 into nucypher:epic-offchain Jul 31, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JsonApiCondition #550

Add JsonApiCondition #550

theref commented Jul 17, 2024

netlify bot commented Jul 17, 2024 •

edited

Loading

netlify bot commented Jul 17, 2024 •

edited

Loading

derekpierre commented Jul 19, 2024

theref commented Jul 19, 2024

derekpierre commented Jul 19, 2024

KPrasch Jul 30, 2024 •

edited

Loading

theref Jul 30, 2024 •

edited

Loading

derekpierre Jul 30, 2024

KPrasch Jul 30, 2024 •

edited

Loading

theref Jul 30, 2024 •

edited

Loading

theref Jul 30, 2024

KPrasch Jul 30, 2024 •

edited

Loading

theref Jul 30, 2024

theref Jul 30, 2024

KPrasch Jul 30, 2024 •

edited

Loading

KPrasch left a comment •

edited

Loading

piotr-roslaniec Jul 30, 2024

theref Jul 31, 2024

	parameters: z.record(z.string(), z.unknown()).optional(),
	parameters: z.record(z.object()).optional(),

Add JsonApiCondition #550

Add JsonApiCondition #550

Conversation

theref commented Jul 17, 2024

netlify bot commented Jul 17, 2024 • edited Loading

✅ Deploy Preview for taco-nft-demo ready!

netlify bot commented Jul 17, 2024 • edited Loading

✅ Deploy Preview for taco-demo ready!

derekpierre commented Jul 19, 2024

theref commented Jul 19, 2024

derekpierre commented Jul 19, 2024

KPrasch Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

theref Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

derekpierre Jul 30, 2024

Choose a reason for hiding this comment

KPrasch Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

theref Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

theref Jul 30, 2024

Choose a reason for hiding this comment

KPrasch Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

theref Jul 30, 2024

Choose a reason for hiding this comment

theref Jul 30, 2024

Choose a reason for hiding this comment

KPrasch Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

KPrasch left a comment • edited Loading

Choose a reason for hiding this comment

piotr-roslaniec Jul 30, 2024

Choose a reason for hiding this comment

theref Jul 31, 2024

Choose a reason for hiding this comment

netlify bot commented Jul 17, 2024 •

edited

Loading

netlify bot commented Jul 17, 2024 •

edited

Loading

KPrasch Jul 30, 2024 •

edited

Loading

theref Jul 30, 2024 •

edited

Loading

KPrasch Jul 30, 2024 •

edited

Loading

theref Jul 30, 2024 •

edited

Loading

KPrasch Jul 30, 2024 •

edited

Loading

KPrasch Jul 30, 2024 •

edited

Loading

KPrasch left a comment •

edited

Loading