feat: handle PgLiteral in index expressions #1561
Conversation
…ns and function calls
Shinigami92
left a comment
Shinigami92
left a comment
There was a problem hiding this comment.
These changes look fantastic and are almost ready to be merged! I've just found a few minor improvements we should make before release.
Also, could you implement a test for the throw-inside-map-loop?
| if (isPgLiteral(col)) { | ||
| throw new Error( | ||
| 'Index name must be provided when using PgLiteral columns' | ||
| ); | ||
| } |
There was a problem hiding this comment.
issue (blocking): we are throwing an error without information about which column was the cause inside a map (loop). This leads to bad UX when running into this case.
| const uniq = 'unique' in options && options.unique ? '_unique' : ''; | ||
|
|
||
| return typeof table === 'object' | ||
| return typeof table === 'object' && 'name' in table |
There was a problem hiding this comment.
suggestion: we have this check (typeof table === 'object' && 'name' in table) now in multiple places. Previously it was just one condition, but now you improved it and made the condition more "complex". With isIndexColumn you already implemented a guard, so I think it is also a good idea to implement a guard for checking if it is a tableName or a schema.
| const OPERATOR_PATTERN = /(->|::)/; | ||
| const LOGICAL_PATTERN = /[=<>!]+/; | ||
| const FUNCTION_CALL_PATTERN = /^[\w.]+\(/; | ||
| const ARITHMETIC_PATTERN = /\s+[+*/-]\s+/; | ||
| const ALPHANUMERIC_PATTERN = /[a-zA-Z0-9]/; |
There was a problem hiding this comment.
thought: I really like this, however deep inside me, I'm a little bit afraid of sql injection 😸
Do you think it is a good idea to implement a negative pattern check for -- (comments) and ; (termination)? These should NEVER be positive results of checks below.
Shinigami92
changed the title
Co-authored-by: Shinigami <chrissi92@hotmail.de>
filmaj
left a comment
filmaj
left a comment
There was a problem hiding this comment.
Thanks for taking this on! <3
Left a couple notes regarding the variety of different json operators available in Postgres.
| readonly shouldQuote: boolean; | ||
| } | ||
|
|
||
| const OPERATOR_PATTERN = /(->|::)/; |
There was a problem hiding this comment.
Worth noting that there are a ton of JSON operators. I think we'll need to include test cases and regex(es) covering operators like #>, @>, ?, ?|, ?&, -, ||, -#.
| ); | ||
| }); | ||
|
|
||
| it('should create index with JSON operator expression', () => { |
There was a problem hiding this comment.
Good call on adding test cases for this. As mentioned in my other comment, I'd also suggest adding test cases that leverage some of the other available JSON operators.
|
@brenoepics just to manage expectations 😉 do you want to work further on the open change requests? or should someone take over? |
Yup! I’ll have some free time tomorrow, so I’m planning to finish the remaining fixes and also add a few extra test cases I noticed. |
fixes #1560