Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

ChrisPdgn
Copy link
Contributor

@ChrisPdgn ChrisPdgn commented Jun 21, 2023

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: Duplicated unique constraints and indexes on sequelize.sync({ alter: true }) 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?

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

Does this PR introduce a breaking change?

  • Yes
  • No

The PR fulfills these requirements:

  • 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:

  • A convincing reason for adding this feature

@ghost
Copy link

ghost commented Jun 30, 2023

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

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@ChrisPdgn ChrisPdgn marked this pull request as ready for review June 30, 2023 08:34
@kon14 kon14 removed the request for review from laertis99 November 20, 2023 13:58
@ChrisPdgn ChrisPdgn marked this pull request as draft July 30, 2024 13:53
refactor(authorization): add default index names
fix(database): import/export checks & ignore indexes of other db type
@ChrisPdgn ChrisPdgn marked this pull request as ready for review August 2, 2024 11:58
@ChrisPdgn ChrisPdgn requested a review from Renc17 August 2, 2024 12:03
Copy link
Contributor

@kkopanidis kkopanidis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. One needed addition, is to create some index compatibility wherever possible. For example mongo ascending and descending index can be converted to the SQL equivalent.

@kkopanidis
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants