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.

Sign up for GitHub

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

Jump to bottom

Api / Fields: Limit constraint name when adding and index to a field #24203

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

Api / Fields: Limit constraint name when adding and index to a field #24203

wants to merge 12 commits into from

Conversation

joselcvarela
Copy link
Member

@joselcvarela joselcvarela commented Dec 12, 2024 •
edited by ComfortablyCoding
Loading

Scope

What's changed:

  • We now limit constraint names (e.g. field index) on a per provider basis via a new schema helper generateIndexName
  • Updated getDefaultIndexName to accept a maxLength to allow configurability on a per db basis
  • is_indexed no longer returns true when only a unique index is set for MSSQL and SQLite

Potential Risks / Drawbacks

  • The util getDefaultIndexName covers the cases when constraint name size is less than 60, but Mysql maximum is 64
    Therefore, indexes with constraint names between 61 and 64 may not be removed using Directus.     No longer applicable with the move to schema helper.
  • Should we need a migration for this? No longer applicable with the move to schema helper.

Review Notes / Questions

  • generateIndexName seemed to be the most appropriate, open to suggestions
  • Limits
    • MYSQL/MariaDB: 64 chars
    • Postgres: 63 chars
    • MSSQL: 128 chars
    • OracleDB: 128 chars. To ensure backwards compatibility with knex if the name exceeds the limit we set the entire thing as base64 encoded hash.
    • SQLite: No limit from what I can find, set maxLength to infinity to ensure backwards compatibility
    • CockroachDB: Recommends 128 chars, currently it is not enforced but may be in the future. As such a limit of 128 char was added due to re-use of pg driver.

TODO

  • Testing
    • confirm limits are correct
    • confirm schema helper resolves the issue for all relevant dbs
    • confirm backwards compatibility for indexes
    • confirm both unique and index still work as expected

Fixes #24202

@joselcvarela joselcvarela requested a review from a team as a code owner December 12, 2024 07:17
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 649bf86

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ComfortablyCoding
ComfortablyCoding reviewed Dec 12, 2024
View reviewed changes
Copy link
Contributor

@ComfortablyCoding ComfortablyCoding left a comment

Choose a reason for hiding this comment

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

I believe the unique and primary constraint(s) have the same issue? We should ensure they are all inline.

@joselcvarela
Copy link
Member Author

I believe the unique and primary constraint(s) have the same issue? We should ensure they are all inline.

Not sure 🤔
It hasn't been an issue so far and we have primary keys and foreign keys for a while, but the index feature is relatively new which means it can have not covered use cases.
@licitdev what you think?

@ComfortablyCoding
Copy link
Contributor

ComfortablyCoding commented Dec 12, 2024 •
edited
Loading

I believe the unique and primary constraint(s) have the same issue? We should ensure they are all inline.

Not sure 🤔 It hasn't been an issue so far and we have primary keys and foreign keys for a while, but the index feature is relatively new which means it can have not covered use cases. @licitdev what you think?

We can probably leave primary and foreign keys alone as they do not seem to have this issue (foreign keys account for it and primary key sets it to PRIMARY).unique on the other hand errors for me with a large enough table name (e.g. this_is_a_really_long_name_to_test_mysql_column_name_char_limit).

Should we need a migration for this?

Ideally we would not, any way we can use the existing index name instead of re-generating it on removal? That way these changes would be backwards compatible and would resolve not being able to remove index names above 60.

cc @licitdev

@joselcvarela
Copy link
Member Author

Ideally we would not, any way we can use the existing index name instead of re-generating it on removal? That way these changes would be backwards compatible and would resolve not being able to remove index names above 60.

How can we retrieve the existing constraint name for that field? One of the solutions would be to rely on the knex convention which is table_field_index but not sure if that will always be the case 🤔

@ComfortablyCoding
Copy link
Contributor

ComfortablyCoding commented Dec 18, 2024 •
edited
Loading

Ideally we would not, any way we can use the existing index name instead of re-generating it on removal? That way these changes would be backwards compatible and would resolve not being able to remove index names above 60.

How can we retrieve the existing constraint name for that field? One of the solutions would be to rely on the knex convention which is table_field_index but not sure if that will always be the case 🤔

Hmm, our current schema helper would be the way but do not believe it currently supports retrieving the index name.

What if we extract this getDefaultIndexName as a generic helper on a per vendor basis? We can update the function to accept an index name limit parameter that defaults to 60 (for backwards compatibility). This way we can re-use it within each vendor with whatever index name limit they support which should resolve both of our issues.

@joselcvarela
Copy link
Member Author

Yeah I think that can actually work. Feel free to adapt this PR 👍

@ComfortablyCoding
Copy link
Contributor

ComfortablyCoding commented Dec 20, 2024 •
edited
Loading

Should be ready for review @joselcvarela / @licitdev

@schtiewen
Copy link

Hi, are there any updates or quickfixes here?

i've the same issue:
MySQL Version 8.

alter table `suggestionInformation_translations` add index `suggestioninformation_translations_suggestioninformation_id_index`(`suggestionInformation_id`) - Identifier name 'suggestioninformation_translations_suggestioninformation_id_index' is too long",

thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ComfortablyCoding ComfortablyCoding ComfortablyCoding left review comments

@licitdev licitdev Awaiting requested review from licitdev

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Api: Not possible to create index on large collection name and field
3 participants
@joselcvarela @ComfortablyCoding @schtiewen