feat: Add option keepUnknownIndexes to retain indexes which are not specified in schema
#9857
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! |
📝 WalkthroughWalkthroughAdds a new SchemaOptions flag Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin/Config
participant Server as Parse Server (SchemaMigration)
participant DB as Database
Admin->>Server: Start migration with SchemaOptions (keepUnknownIndexes = true/false)
Server->>DB: Fetch current DB indexes (cloud schema)
Server->>Server: Compare local schema indexes vs DB indexes
alt keepUnknownIndexes = false
Server->>DB: Drop indexes not defined in local schema
Note right of Server #DDEBF7: Unknown indexes removed
else keepUnknownIndexes = true
Server->>DB: Skip dropping unknown indexes
Note right of Server #F7F3DE: Preserve existing DB indexes
end
Server->>DB: Create/add missing indexes defined in local schema
Server-->>Admin: Migration complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/SchemaMigrations/DefinedSchemas.js (1)
348-372: Use schemaOptions, not server config; avoid creating conflicting indexes when not droppingIndex deletion is gated by this.config.schema.dropUnknownIndexes, but all other migration flags read from this.schemaOptions. This makes per-run options ineffective and can break tests. Also, when dropUnknownIndexes is false, pushing a replacement into indexesToAdd without dropping first will likely fail (index name already exists).
if (cloudSchema.indexes) { + const dropUnknown = this.schemaOptions.dropUnknownIndexes !== false; // default true Object.keys(cloudSchema.indexes).forEach(indexName => { if (!this.isProtectedIndex(localSchema.className, indexName)) { if (!localSchema.indexes || !localSchema.indexes[indexName]) { - // Only delete indexes which are present in database if dropUnknownIndexes is `true` - if(this.config.schema.dropUnknownIndexes){ - newLocalSchema.deleteIndex(indexName); - } + // Delete only when allowed; otherwise warn in strict mode + if (dropUnknown) { + newLocalSchema.deleteIndex(indexName); + } else if (this.schemaOptions.strict === true) { + logger.warn( + `Index "${indexName}" exists in DB for "${localSchema.className}" but is not defined; keeping it because dropUnknownIndexes=false.` + ); + } } else if ( !this.paramsAreEquals(localSchema.indexes[indexName], cloudSchema.indexes[indexName]) ) { - // Only delete indexes which are present in database if dropUnknownIndexes is `true` - if(this.config.schema.dropUnknownIndexes){ - newLocalSchema.deleteIndex(indexName); - } - if (localSchema.indexes) { - indexesToAdd.push({ - indexName, - index: localSchema.indexes[indexName], - }); - } + // Replace only when allowed; otherwise warn and leave as-is + if (dropUnknown) { + newLocalSchema.deleteIndex(indexName); + if (localSchema.indexes) { + indexesToAdd.push({ + indexName, + index: localSchema.indexes[indexName], + }); + } + } else if (this.schemaOptions.strict === true) { + logger.warn( + `Index "${indexName}" differs for "${localSchema.className}" but dropUnknownIndexes=false; leaving existing index unchanged.` + ); + } } } }); }spec/DefinedSchemas.spec.js (1)
374-395: Test name and setup inverted; make it explicitly exercise “true” pathTitle says “false” but the behavior verified is the “true” path. Also make the flag explicit in the options object for clarity.
- it('should delete removed indexes when dropUnknownIndexes is set to false', async () => { + it('should delete removed indexes when dropUnknownIndexes is true', async () => { const server = await reconfigureServer(); let indexes = { complex: { createdAt: 1, updatedAt: 1 } }; - let schemas = { definitions: [{ className: 'Test', indexes }] }; + let schemas = { definitions: [{ className: 'Test', indexes }], dropUnknownIndexes: true }; await new DefinedSchemas(schemas, server.config).execute(); indexes = {}; - schemas = { definitions: [{ className: 'Test', indexes }] }; + schemas = { definitions: [{ className: 'Test', indexes }], dropUnknownIndexes: true };
🧹 Nitpick comments (2)
src/Options/index.js (1)
28-30: Polish wording; fix grammar and clarity of the new option descriptionTighten phrasing and fix “wont” → “won’t”. Also prefer “during schema migrations.”
- /* Drops indexes that are not defined in the schema and are present in the database. Set this false if you are adding indexes manually so that it wont be dropped when you run schema migration + /* Drops indexes that are not defined in the schema but exist in the database. Set this to false if you add indexes manually so they won't be dropped during schema migrations. :DEFAULT: true */ dropUnknownIndexes: ?boolean;src/Options/docs.js (1)
7-7: Fix grammar in docs for dropUnknownIndexesUse “Set this to false … won’t … during schema migrations.”
- * @property {Boolean} dropUnknownIndexes Drops indexes that are not defined in the schema and are present in the database. Set this false if you are adding indexes manually so that it wont be dropped when you run schema migration + * @property {Boolean} dropUnknownIndexes Drops indexes that are not defined in the schema but exist in the database. Set this to false if you add indexes manually so they won't be dropped during schema migrations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
spec/DefinedSchemas.spec.js(2 hunks)src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(1 hunks)src/SchemaMigrations/DefinedSchemas.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
spec/DefinedSchemas.spec.js (2)
spec/helper.js (2)
reconfigureServer(171-205)Parse(4-4)src/SchemaMigrations/DefinedSchemas.js (1)
Parse(3-3)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 30-30: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: Docker Build
🔇 Additional comments (2)
src/Options/index.js (1)
28-30: Configure Biome to parse Flow or exclude Flow-typed files
There’s no Biome config in the repo, so it’s using defaults that don’t recognize Flow’s “?boolean” syntax and error on the “?” token. Add a Biome config (for example a.biome.jsonorbiome.config.js) with"language": "flow", or explicitly ignore the Flow-typed file pattern (e.g."ignore": ["src/Options/*.js"]) so the Flow annotation isn’t parsed by Biome.src/Options/Definitions.js (1)
31-37: Fix help text typo and clarify phrasing in source Options/index.js
Update thedropUnknownIndexeshelp string in Options/index.js (not Definitions.js) and re-run the generator so the updated text propagates. Change “wont” to “won’t” and rephrase to:
"Drops indexes that are not defined in the schema but exist in the database. Set this to false if you add indexes manually so they won't be dropped during schema migrations."
RahulLanjewar93
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
RahulLanjewar93
changed the title
dropUnknownIndexes to conditionally delete indexes if not specified in schema
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
dropUnknownIndexes to conditionally delete indexes if not specified in schemadropUnknownIndexes to conditionally delete indexes if not specified in schema
RahulLanjewar93
force-pushed
the
feat-dropUnknownIndexes
branch
from
84cf02c to
de436f0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #9857 +/- ##
==========================================
+ Coverage 92.99% 93.02% +0.02%
==========================================
Files 187 187
Lines 15096 15097 +1
Branches 174 174
==========================================
+ Hits 14039 14044 +5
+ Misses 1045 1041 -4
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Options/index.js (1)
28-30: Polish the docstring; fix grammar and match existing option style.Short, consistent phrasing; fix “wont” → “won’t”; clarify “Set to false…”.
- /* Drops indexes that are not defined in the schema and are present in the database. Set this false if you are adding indexes manually so that it wont be dropped when you run schema migration - :DEFAULT: true */ + /* Is true if Parse Server should drop database indexes that are present but not defined in the schema. Set to `false` to retain manually created indexes during schema migrations. + :DEFAULT: true */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(1 hunks)src/SchemaMigrations/DefinedSchemas.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Options/docs.js
- src/SchemaMigrations/DefinedSchemas.js
- src/Options/Definitions.js
🧰 Additional context used
🪛 Biome (2.1.2)
src/Options/index.js
[error] 30-30: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: Node 18
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Docker Build
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (1)
src/Options/index.js (1)
30-30: Biome parse hint is likely a false positive on Flow’s?boolean.If CI gates on Biome, consider excluding Flow type files or enabling a Flow-compatible parser for this path. No code change needed.
|
@mtrezza what do you think? should we approach this problem some other way? |
|
@Moumouls is the expert when it comes to defined schema, what do you think? |
There was a problem hiding this comment.
issue: the new option should be inverted, otherwise LGTM, good idea, and thanks for the PR @RahulLanjewar93 🚀
and the approach is correct !
Thanks, will make the changes. There is one another bug, if we add some indexes, run migration it works fine, but if we remove those indexes and run migration it does not update the corresponding So we have two approaches now
|
|
I think the current approach is safe and simple. The real core issue is Defined Schema do not support creation of unique/ttl indexes (or other kind of indexes) A great goal could be to implement a way to provide advanced indexes through Defined Schema, but i'm not 100% sure of the correct approach at this time So the mode currently is:
|
RahulLanjewar93
changed the title
dropUnknownIndexes to conditionally delete indexes if not specified in schemakeepUnknownIndexes to retain indexes which are not specified in schema
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Options/index.js (1)
28-30: Fix minor grammar in option description.“wont” → “won’t”; tighten phrasing.
- /* Keep indexes that are not defined in the schema and are present in the database. Set this to true if you are adding indexes manually so that it wont be dropped when you run schema migration + /* Keep database indexes that are not defined in the schema. Set this to true if you add indexes manually so that they won't be dropped during schema migration. :DEFAULT: false */ keepUnknownIndexes: ?boolean;src/Options/Definitions.js (1)
31-37: Polish help text (grammar/clarity).Same typo and phrasing as in the interface doc.
keepUnknownIndexes: { env: 'PARSE_SERVER_SCHEMA_KEEP_UNKNOWN_INDEXES', - help: - 'Keep indexes that are not defined in the schema and are present in the database. Set this to true if you are adding indexes manually so that it wont be dropped when you run schema migration', + help: + "Keep database indexes that are not defined in the schema. Set this to true if you add indexes manually so that they won't be dropped during schema migration.", action: parsers.booleanParser, default: false, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
spec/DefinedSchemas.spec.js(2 hunks)src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(1 hunks)src/SchemaMigrations/DefinedSchemas.js(1 hunks)src/SchemaMigrations/Migrations.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/SchemaMigrations/DefinedSchemas.js
- src/Options/docs.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
spec/DefinedSchemas.spec.js (2)
spec/helper.js (2)
reconfigureServer(171-205)Parse(4-4)src/SchemaMigrations/DefinedSchemas.js (1)
Parse(3-3)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 30-30: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
src/SchemaMigrations/Migrations.js
[error] 9-9: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Docker Build
🔇 Additional comments (4)
src/SchemaMigrations/Migrations.js (1)
9-9: ApprovekeepUnknownIndexesaddition
No staledropUnknownIndexesreferences found;keepUnknownIndexesis defined and used consistently in Options (index.js, docs.js, Definitions.js), Migrations.js, DefinedSchemas.js, and associated tests.spec/DefinedSchemas.spec.js (3)
374-396: Default behavior test reads well.Covers the “unset” (default false) path; assertions are precise after cleaning
_id_.
397-418: Explicit false path test is correct.Good coverage mirroring the default; expectations are strict and deterministic.
420-441: True path preservation test is solid.Accurately validates preservation when
keepUnknownIndexesis true.
Have added the required changes |
Anyime mate |
|
I think we are all good here @mtrezza ! |
Pull Request
Issue
Deletes additional indexes which are not specified in schema.
Closes: #9112
Approach
We are always deleting the indexes in the db if they are not present in the provided schema definition.
However that may not be the case since the user may add addtional indexes as parse does not support all kinds of indexes. To retain such indexes, provided an option
keepUnknownIndexesTasks
Summary by CodeRabbit
New Features
Documentation
Tests