allow all numbers in jwt token type names #511
Conversation
|
O_o this seems weird; surely that'd have to be deliberate... @calebmer any idea? |
|
right? but I couldn't think of one. |
|
Tracking through various levels of git blame; this is where it was originally introduced:
Worrisome... I wonder what's tricky about them. I had a scan through the docs and can't see any issues: https://www.postgresql.org/docs/9.6/static/sql-createtype.html Could you add some tests so we can be certain? |
|
will do. |
|
Let me know if I've completely missed on your conventions - I'm new to ES6, TypeScript, Jest, Travis, and ESLint. Also - sorry this is another suite of slow tests in the postgraphql space. |
| let badQuery = 'not an error' | ||
|
|
||
| try { | ||
| badQuery = await client.query(`create schema ${badName}; create type ${badName}.jwt_token as (role text, exp integer, a integer, b integer, c integer)`) |
There was a problem hiding this comment.
I think the reason this fails is just that you've not escaped the identifier with double quotes - that doesn't make it inherently invalid. Does it fail if you add double quotes?
There was a problem hiding this comment.
create schema "0_test";
CREATE SCHEMA
Time: 35.303 ms
There was a problem hiding this comment.
you're right. works fine when escaped.
|
|
||
| // good names start with a letter or underscore and may also contain numbers after position 0 | ||
| const goodNames = [ | ||
| 'test', 'test_0', 'test_1', 'test_2', 'test_3', 'test_4', 'test_5', 'test_6', 'test_7', 'test_8', 'test_9', '_test', |
There was a problem hiding this comment.
This only really needs to test edge cases, so 'test_0', 'test_9' and '_test' should suffice
|
|
||
| // schema, type, etc... names are not allowed to start with numbers | ||
| const badNames = [ | ||
| '0_test', '1_test', '2_test', '3_test', '4_test', '5_test', '6_test', '7_test', '8_test', '9_test', |
There was a problem hiding this comment.
similarly I'd only test '0_test' and '9_test'
…ping to testing of names starting with a number
|
@gkchestertron Are you good for this to be merged? |
|
works for me @benjie |
* allow all numbers in jwt token type names * fix(postgraphql): improve jwt token type check regex and add tests for it * fix(postgraphql): clean up bad style * fix(postgraphql) more style fixes * fix(postgraphql) removes unnecessary tests and adds double-quote escaping to testing of names starting with a number
currently naming of the jwt_token type, as in the tutorial/example only allows for names that contain the numbers 1 and 2. I'm using this for multiple PTA organizations and would like to keep consistent naming and identify them by their district number (i.e. pta_dist_14.jwt_token).
This fixes the issue - but I am unsure if there was a good reason that only 1 and two were allowed before.