Conduit icon indicating copy to clipboard operation
Conduit copied to clipboard

feat(database)!: add index support for all sql dialects & index refactoring

Open ChrisPdgn opened this issue 2 years ago • 2 comments

This PR adds the full index support that was missing for MySQL, MariaDB and SQLite

Breaking changes:

  • getDatabaseType() no longer returns 'PostgreSQL', but the sequelize dialect string 'postgres'
  • ModelOptions index types field in sequelize now has the type of Array like in mongoose (even though sequelize requires only one index type and not one for each index field like in mongoose)
  • name is now a required field for indexes, as it ensures uniqueness

Fixes:

  • A few mongo indexes where added in some authz schemas (ActorIndex, ObjectIndex, Permission, Relationship) while this PR wasn't merged yet, so I had to add the default names mongoose assigns to indexes without a specified name. Meanwhile, I had to add some logic in schema converters to ignore indexes of another DB type, so that these indexes wouldn't cause an issue in case of using conduit with another DB like SQL

  • Bugs associated with indexes not being stored inside schema (field indexes + modelOptions indexes) when using the endpoints for index creation/deletion

  • indexes key missing in validateModelOptions() not allowing schemas that contain modelOptions indexes to be patched

Refactoring:

  • Some needed cleanup
  • Clear & specified params for index endpoints

Additions:

  • Import/export indexes endpoints

Notes:

  • The index duplication bug was not encountered during testing. The created unique indexes are not duplicated, but duplication of constraints was noticed when syncing schemas with alter: true (indexes apparently don't have anything to do with it as it is a bug the exists anyway). Probably, we will wait for sequelize version 7 that fixes this bug. Reference: sequelize/sequelize#12889
  • As there is no such thing as index editing/updating, when importing indexes, if there is an already existing index with the same name as with an imported one, the imported index is ignored and the already existing one is kept

What kind of change does this PR introduce?

  • [x] Bugfix
  • [x] Feature
  • [ ] Code style update
  • [x] Refactor
  • [ ] Build-related changes
  • [ ] Other (please describe)

Does this PR introduce a breaking change?

  • [x] Yes
  • [ ] No

The PR fulfills these requirements:

  • [x] It's submitted to the main branch
  • [ ] When resolving a specific issue, it's referenced in the PR's description (e.g. fix #xxx, where "xxx" is the issue number)

If adding a new feature, the PR's description includes:

  • [x] A convincing reason for adding this feature

ChrisPdgn avatar Jun 21 '23 09:06 ChrisPdgn

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

ghost avatar Jun 30 '23 08:06 ghost

Perhaps it makes sense to have a set of compatible indexes with a distinct type, so that when we use them in platform models we know they are going to be OK in all deployments

kkopanidis avatar Aug 06 '24 09:08 kkopanidis