Fix JSON API conditions #602
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## epic-v0.6.x #602 +/- ##
==============================================
Coverage ? 89.27%
==============================================
Files ? 73
Lines ? 6545
Branches ? 351
==============================================
Hits ? 5843
Misses ? 664
Partials ? 38 ☔ View full report in Codecov by Sentry. |
* would match 0 - infinity Co-authored-by: KPrasch <[email protected]>
There was a problem hiding this comment.
@theref - left some additional RFCs still needed even though this PR was already merged.
Please follow-up on those.
Side note: are releases necessary? A different option is to use local dependency installations and make fixes locally as you need, and then do one PR into the EPIC.
eg.
$ npm install <path_to_local_taco_web>/taco-web/packages/taco
$ npm install <path_to_local_taco_web>/taco-web/packages/taco-auth
Of course, pnpm can be used instead of npm depending on what the demo you are testing with uses.
This would prevent needing to do a release after every fix you make.
| } | ||
| } | ||
| } | ||
| if (this.isContextParameter(condition.authorizationToken)) { |
There was a problem hiding this comment.
An authorizationToken is optional and can only be a context parameter.
| if (this.isContextParameter(condition.authorizationToken)) { | |
| if (condition.authorizationToken) { |
| } | ||
| } | ||
| if (condition.query) { | ||
| const queryParams = condition.query.match(":[a-zA-Z_]+"); |
There was a problem hiding this comment.
There is an existing regex for context variables - CONTEXT_PARAM_REGEXP in const.ts that you can use. You should use that one instead. Context parameters allow numbers to be in the string (just not the first character).
There was a problem hiding this comment.
@theref discovered some nuance in the existing regex (CONTEXT_PARAM_REGEXP) that does not fully cover the Json API condition use case. In particular the CONTEXT_PARAM_REGEXP enforces that the pattern must match at the start of a string (^), however, it's possible that a context parameter may appear as a substring in the middle of an endpoint (for example https://example.com/:userAddress/balance). We think this is specific to the case of JSON API conditions, but let us know if you think otherwise.
There was a problem hiding this comment.
I'd also like to point out a few other concerns I have about CONTEXT_PARAM_REGEXP : ^:[a-zA-Z_][a-zA-Z0-9_]*$:
- The usage of the
*character here permits matching zero tokens, which means that a simple:will match positively. Instead this can be a+to match one or more tokens. - Are we intending to support purely numeric context params?
There was a problem hiding this comment.
There are two context regexes in const.ts.
- CONTEXT_PARAM_REGEXP - matches a regex without expecting it to be at the beginning of the string i.e. no caret.
- CONTEXT_PARAM_FULL_MATCH_REGEXP - expects the entire string to be a context variable
I believe (?) the first one should be sufficient here.
There was a problem hiding this comment.
I'd also like to point out that there is a small bug in
CONTEXT_PARAM_REGEXP:^:[a-zA-Z_][a-zA-Z0-9_]*$. The usage of the*character here permits matching zero tokens, which means that a simple:will match positively.
It's subtle, but the [a-zA-Z_] part (first block) ensures that the context variable starts with a letter or underscore, the rest of regex allows the remaining characters to be a letter, number, or underscore.
There was a problem hiding this comment.
It's subtle, but the [a-zA-Z_] part (first block) ensures that the context variable starts with a letter or underscore, the rest of regex allows the remaining characters to be a letter, number, or underscore.
Ah yes, you are correct!
There was a problem hiding this comment.
There are two context regexes in
const.ts.
- CONTEXT_PARAM_REGEXP - matches a regex without expecting it to be at the beginning of the string i.e. no caret.
- CONTEXT_PARAM_FULL_MATCH_REGEXP - expects the entire string to be a context variable
I believe (?) the first one should be sufficient here.
Hmm, perhaps it's in another branch? I don't see two, only CONTEXT_PARAM_REGEXP in const.ts
There was a problem hiding this comment.
|
PS. Nice find @theref - this was definitely missing code in the requested parameters logic. |
Context params where not being parsed from endpoint, query, or authToken. This PR fixes those issues and adds a test