fix: Improve Scope of Table Name Regex to Resolve Errors #188
Conversation
There was a problem hiding this comment.
Mostly looks good, but for now I think we should remove the errors that are thrown when trying to create context.db. Instead we should just log the errors and not add it to the context. This avoids us breaking existing indexers unintentionally. We can be more strict once we have the final solution.
runner/src/indexer/indexer.test.ts
Outdated
| CREATE TABLE | ||
| creator_quest ( |
There was a problem hiding this comment.
Do we want to test same line too?
There was a problem hiding this comment.
That's a good idea. I'll make this one one line to validate that. And I agree with the overall comment. I'll make it return an empty object if it detects a failure for now.
There was a problem hiding this comment.
This was a great catch. Turns out the regex would have \s+ twice. So, the space between TABLE and the name would only satisfy one and it would be skipped.
runner/src/indexer/indexer.test.ts
Outdated
| 'insert__contractor___quest_', | ||
| 'select__contractor___quest_', |
There was a problem hiding this comment.
Why is there a _ at the end?
There was a problem hiding this comment.
This is an artifact of some edge case I had considered from an earlier implementation. The outside _ are where the double quote symbol was replaced. The current implementation adds quotes after the table names are collected. I had an edge case in mind where there might be "tableName" and tableName and we would want to distinguish between them but I think the current code will fetch only tableName as it does not require double quotes since its a valid name on its own. If someone wanted "tableName" as the actual name, it'd have to be ""tableName"" or something. In any case, I can strip the outer quotes before doing the replacement.
runner/src/indexer/indexer.ts
Outdated
| await this.writeLog(`context.db.insert_${tableName}`, blockHeight, `Calling context.db.insert_${tableName}.`, `Inserting object ${JSON.stringify(objects)} into table ${tableName} on schema ${schemaName}`); | ||
| [`insert_${tableName.replace(/[^a-zA-Z0-9_]/g, '_')}`]: async (objects: any) => { | ||
| await this.writeLog(`context.db.insert_${tableName.replace(/[^a-zA-Z0-9_]/g, '_')}`, blockHeight, | ||
| `Calling context.db.insert_${tableName.replace(/[^a-zA-Z0-9_]/g, '_')}.`, |
There was a problem hiding this comment.
| `Calling context.db.insert_${tableName.replace(/[^a-zA-Z0-9_]/g, '_')}.`, | |
| `Calling context.db.insert_${this.sanitizeTableName(tableName)}.`, |
This is used all over the place - maybe extract to its own method?
The previous regex was not able to support DDL where the table names were not encapsulated in quotes. Through more research, I learned as well that putting the table name in quotes causes the table name it be case sensitive and also allow special characters. I also omitted checking for "IF NOT EXISTS". So, I updated the regex to allow optional white space and "IF NOT EXISTS". Then, I skip a SINGLE optional double quote, match anything until another optional double quote or white space. Everything between the white space following either TABLE or EXISTS and potentially two double quotes is now the table name. The name can have special characters so I ensure that any such names have double quotes added back in. I also support table names defined by schema.tableName. Finally, for testing I did the following:
Tested generating table names in front end by opening all indexers and running debug mode to generate methods. All indexers succeeded, including notable ones like bos_quests, social_feed, and widget_testing.
I also added a stress test schema which is a valid DDL schema (verified using format button) which contains all kinds of edge cases, and ensure the generated method names correctly sanitize the special characters to underscores. There might be other ways to more cleanly implement this transition of special characters to function name but I think this is fine for now, and we hopefully will have autocomplete for the context object soon.
Finally, I also did an integration test using the existing insert function to confirm functionality has remained unimpacted.