feat: Automatically create indexes for _Session.sessionToken and class relations
#8346
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
Thanks for opening this pull request!
|
mtrezza
changed the title
sessionToken and relations
mtrezza
changed the title
sessionToken and relations_Session.sessionToken and relations
mtrezza
changed the title
_Session.sessionToken and relations_Session.sessionToken and class relations
Codecov ReportBase: 94.04% // Head: 93.86% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## alpha #8346 +/- ##
==========================================
- Coverage 94.04% 93.86% -0.18%
==========================================
Files 181 181
Lines 14266 14303 +37
==========================================
+ Hits 13416 13426 +10
- Misses 850 877 +27
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Nice PR @dblythy ! 🚀
But we do not have in Parse Server a single source of truth about default indexes (not like systemClasses from schema controller).
You need to add the session index to isProtectedIndex function in DefinedSchema and update tests to ensure that if Session class is used in DefinedSchema, the _session_token is not deleted.
Note: we don't need to protect join classes index since it's internal classes and should not be defined in a DefinedSchema
|
@Moumouls Do you want to review this before we merge? |
| (async () => { | ||
| if (this._relationTablesAdded.includes(className)) { | ||
| return; | ||
| } | ||
| const exists = await this.collectionExists(className); | ||
| const names = []; | ||
| if (exists) { | ||
| const indexes = (await this.adapter.getIndexes(className)) || []; | ||
| names.push( | ||
| ...indexes.map(({ name, indexname }) => { | ||
| if (name) { | ||
| return name.split('_')[0]; | ||
| } | ||
| const splitName = indexname.split('_'); | ||
| return splitName.at(-1); | ||
| }) | ||
| ); | ||
| } | ||
| const keys = ['relatedId', 'owningId']; | ||
| await Promise.all( | ||
| keys.map(async subKey => { | ||
| if (names.includes(subKey)) { | ||
| return; | ||
| } | ||
| try { | ||
| await this.adapter.ensureIndex(className, relationSchema, [subKey]); | ||
| } catch (error) { | ||
| if (error.code !== '23505') { | ||
| logger.warn('Unable to create relatedId index: ', error); | ||
| } | ||
| } | ||
| }) | ||
| ); | ||
| this._relationTablesAdded.push(className); | ||
| })(); |
There was a problem hiding this comment.
I'm not a huge fan of "floating promises" because in case of integration testing it could lead to open connections issues. It's a one-shot operation, we could create a cache to store the indexPromise, then other calls for the same relation could await a unique promise. All parallel calls on the same server will also avoid a spam to the index creation api, since only One promise will be created
Index creation is fast (DB do not wait for indexes created for all records). the promise index creation could be wrapped with a try-catch, so even if the index creation fail, the relation will be created.
| relation.add(obj); | ||
| await obj2.save(); | ||
| await obj2.destroy(); | ||
| await new Promise(resolve => setTimeout(resolve, 1000)); |
There was a problem hiding this comment.
We should avoid hard coded timeout, it's flaky
here a simple function that you can use to add to the utils test suite (it's TS, you can convert to JS)
export const waitFor = async (
fn: () => Promise<any>,
count = 0,
timeout = 100,
): Promise<undefined> => {
try {
await fn()
return undefined
} catch (e: any) {
await new Promise((resolve) => setTimeout(resolve, timeout))
if (count < 100) {
return waitFor(fn, count + 1, timeout)
}
throw e
}
}And usage
await waitFor(async () => {
expect(requestSpy).toHaveBeenCalledTimes(3)
})Wait for will resolve asap and re execute the provided fn until the fn success.
| createJoinTable(className: string) { | ||
| const startupPromise = async () => { | ||
| if (this._relationTablesCache.classes.includes(className)) { | ||
| return; | ||
| } | ||
| const exists = await this.collectionExists(className); | ||
| const names = []; | ||
| if (exists) { | ||
| const indexes = (await this.adapter.getIndexes(className)) || []; | ||
| names.push( | ||
| ...indexes.map(({ name, indexname }) => { | ||
| if (name) { | ||
| return name.split('_')[0]; | ||
| } | ||
| const splitName = indexname.split('_'); | ||
| return splitName.at(-1); | ||
| }) | ||
| ); | ||
| } | ||
| const keys = ['relatedId', 'owningId']; | ||
| await Promise.all( | ||
| keys.map(async subKey => { | ||
| if (names.includes(subKey)) { | ||
| return; | ||
| } | ||
| try { | ||
| await this.adapter.ensureIndex(className, relationSchema, [subKey]); | ||
| } catch (error) { | ||
| if (error.code !== '23505') { | ||
| logger.warn('Unable to create relatedId index: ', error); | ||
| } | ||
| } | ||
| }) | ||
| ); | ||
| this._relationTablesCache.classes.push(className); | ||
| }; | ||
| const promise = this._relationTablesCache.promises[className] || startupPromise(); | ||
| this._relationTablesCache.promises[className] = promise; | ||
| return promise; | ||
| } | ||
|
|
There was a problem hiding this comment.
| createJoinTable(className: string) { | |
| const startupPromise = async () => { | |
| if (this._relationTablesCache.classes.includes(className)) { | |
| return; | |
| } | |
| const exists = await this.collectionExists(className); | |
| const names = []; | |
| if (exists) { | |
| const indexes = (await this.adapter.getIndexes(className)) || []; | |
| names.push( | |
| ...indexes.map(({ name, indexname }) => { | |
| if (name) { | |
| return name.split('_')[0]; | |
| } | |
| const splitName = indexname.split('_'); | |
| return splitName.at(-1); | |
| }) | |
| ); | |
| } | |
| const keys = ['relatedId', 'owningId']; | |
| await Promise.all( | |
| keys.map(async subKey => { | |
| if (names.includes(subKey)) { | |
| return; | |
| } | |
| try { | |
| await this.adapter.ensureIndex(className, relationSchema, [subKey]); | |
| } catch (error) { | |
| if (error.code !== '23505') { | |
| logger.warn('Unable to create relatedId index: ', error); | |
| } | |
| } | |
| }) | |
| ); | |
| this._relationTablesCache.classes.push(className); | |
| }; | |
| const promise = this._relationTablesCache.promises[className] || startupPromise(); | |
| this._relationTablesCache.promises[className] = promise; | |
| return promise; | |
| } | |
| ensureJoinTable(className: string) { | |
| const classNamePromise = this._relationTablesCache[className] | |
| if(classNamePromise) return classNamePromise | |
| const startupPromise = async () => { | |
| const exists = await this.collectionExists(className); | |
| const names = []; | |
| if (exists) { | |
| const indexes = (await this.adapter.getIndexes(className)) || []; | |
| names.push( | |
| ...indexes.map(({ name, indexname }) => { | |
| if (name) { | |
| return name.split('_')[0]; | |
| } | |
| const splitName = indexname.split('_'); | |
| return splitName.at(-1); | |
| }) | |
| ); | |
| } | |
| const keys = ['relatedId', 'owningId']; | |
| await Promise.all( | |
| keys.map(async subKey => { | |
| if (names.includes(subKey)) { | |
| return; | |
| } | |
| try { | |
| await this.adapter.ensureIndex(className, relationSchema, [subKey]); | |
| } catch (error) { | |
| if (error.code !== '23505') { | |
| logger.warn('Unable to create relatedId index: ', error); | |
| } | |
| } | |
| }) | |
| ); | |
| this._relationTablesCache.classes.push(className); | |
| }; | |
| const promise = startupPromise() | |
| this._relationTablesCache[className] = promise | |
| return promise | |
| } | |
Too many source of truth just use a signe object with indexed promises by class names and early return
Code not tested but the strategu should work
New Pull Request Checklist
Issue Description
Parse Server does not create indexes on the
_Sessionclass and join tables.Closes: #8290
Closes: #7832
Approach
Adds indexes
TODOs before merging