balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.85k stars 1.95k forks source link

mn-table name too long #5443

Closed nichdiekuh closed 5 years ago

nichdiekuh commented 10 years ago

Hello,

the way waterline builds association table names quickly exceeds the max table name length in mysql - it's just 64 characters.

For instance: I have two models with the following names:

Both models have an mn-association and the resulting tablename is:

 ER_WRONG_TABLE_NAME: Incorrect table name 'interpretationparameter_parametervalueinterpretations__parametervalueinterpretation_interpretationpa'

I think this naming scheme is quite wasteful and should be changed. As soon as both table names combined are >30 characters, the creation of mn-tables will fail.

My proposal : mn__table_table2

victor-carv commented 10 years ago

I had the same problem with mongodb/sails-mongo.

devinivy commented 10 years ago

I agree that this is a bug, but perhaps this should be moved to waterline-schema? This can also be remedied by using through-associations, which work as described in #705 (on v0.10.12).

Bear in mind that there could be several many-to-many relationships between two resources, so the naming issue isn't easily fixed–especially while keeping the name descriptive.

particlebanana commented 10 years ago

Yup this is tricky. The reason for the crazy join table names is because you can have multiple many-to-many relationships between models. So the table describes the relationship between the models and which attributes its connecting. The longer the model name and attribute name the longer the table name. We can't simply cut it off because you could end up with two tables that have the same name.

A possible solution is to map all joins between two models in a single table and limit model name length. This would give us a join table with x columns depending on how many relationships the two models have. This would break all the current 0.10.x apps and be a pain to migrate though. The other solution is to enforce a limit on model and attribute name lengths. This should probably be added anyway if we map attribute names to column names because there are limits there too.

particlebanana commented 10 years ago

Open to ideas on quick solutions and longer term solutions.

nichdiekuh commented 10 years ago

I made my proposal above ^^ `mn__table1_table2``

devinivy commented 10 years ago

But that naming convention is table-specific and not association-specific (in the case that the same two collections have multiple many-to-may relationships). I'd suggest that the name of the pivot table be able to be specified on the dominant side of the association, directly on the attribute.

nichdiekuh commented 10 years ago

I think that's what "through-associations" are made for. IMO mn_table_table is sufficient for most cases and if you really need to create a second association for the same tables, both model attributes should define a name using through.

devinivy commented 10 years ago

I disagree–I'd much rather multiple many-to-many associations be supported between two collections than table name length be supported for particular adapters. (An aside... I believe the adapters themselves could actually adjust for this by transforming the table name!) Through-associations are made to customize your join table or to attach additional information to the relationship between two collections. A perfect example of such a customization would be to specify the pivot table's name. In any case, I think the relevant feature here would be to allow configuring the pivot table name on the dominant attribute of the association.

nichdiekuh commented 10 years ago

Which table is "dominant" in a mn-association? Aren't they both equal?

I think the bottom line is: As long as the pivot-table name would be (optionally) configurable and the default name shorter, it should be fine.

And as a side node: In Sequelize it's quite similar. The default table is Table1Table2 and the name can be changed for multiple mn-associations between two tables.

devinivy commented 10 years ago

Waterline is adapter-based and collections can span multiple connections/adapters. One side of the association is considered dominant if its collection and the relevant pivot table exist on the same connection. There is an attribute option for this that was considered to be required for a while. Now I believe it's optional if both primary tables are on the same connection.

The change you're suggesting would be a breaking change, which is primarily why I'm suggesting other options, though it is really nice to know how Sequelize dealt with the issue. Curious!

tjwebb commented 10 years ago

Why has no one mentioned the obvious weirdness of defining a model called ParameterValueInterpretations? It looks like a join table; in all the semantic scenarios I can conjure, you'd want a model called Interpretation, and a model called ParameterValue, and you'd relate the two with a many-to-many association.

This isn't so much a bug as the exploitation of a known edge case. For example there may exist some database somewhere that doesn't even support underscores in table names at all; it'd be up to the adapter to sanitize that input. I think its reasonable to expect the sails-mysql adapter to throw an error specific to this 64-char constraint, or fix it on-the-fly with a warning.

nichdiekuh commented 10 years ago

It's hard to explain how we ended up with those table names (there are already tables called Interpretationand ParameterValue), but regardless of my table-naming-scheme the issue remains. The way the mn-table names are build can quickly exceed 64characters.

tjwebb commented 10 years ago

I'm saying that 64 characters is a mysql limit, so that adapter should handle this edge case. If it doesn't, then that's a bug.

nichdiekuh commented 10 years ago

Ok, for the sake of completeness, regarding the comment by @victor0402

http://docs.mongodb.org/manual/reference/limits/

Namespace Length
Each namespace, including database and collection name, must be shorter than 123 bytes.
devinivy commented 10 years ago

I agree that this is the adapter's job. The adapter can look at the schema and make its own decisions. If we want to get crazy, we could let the relevant adapter hook into waterline-schema for this sort of job. I still maintain that a great feature would be for the dominant attribute in a many-to-many to be able to specify the pivot table's name!

dmarcelino commented 9 years ago

On this matter I agree with both @tjwebb:

I'm saying that 64 characters is a mysql limit, so that adapter should handle this edge case. If it doesn't, then that's a bug.

and @devinivy:

I still maintain that a great feature would be for the dominant attribute in a many-to-many to be able to specify the pivot table's name!

The join tables names can stay as they are and the user should be able to provide a custom name. For instance, in a GraphDB, where there are lots of associations and join tables are really edges I would like to see meaningful edge names.

dmarcelino commented 9 years ago

My suggestion regarding waterline core changes is for it to support a new parameter named joinTableName within the association attribute like below:

{
  tableName: 'driverTable',
  identity: 'driver',

  attributes: {
    taxis: {
      collection: 'taxi',
      via: 'drivers',
      joinTableName: 'drives',  // sets junction tableName without affecting its identity
      dominant: true
    },
}

PS: I've actually tried to implement this via adapter (waterline-orientdb) but I got errors like the below:

  Association Interface n:m association :: .add() create nested associations() with single level depth and objects should create a new driver and taxis and associate them:
     Error: Unknown rule: joinTableName
      at Object.matchRule (/Users/warchild/Development/node/waterline-orientdb/node_modules/waterline/node_modules/anchor/lib/match/
matchRule.js:37:11)
mkeremguc commented 9 years ago

can we discuss on a solution to this ?.. btw, it seems that postgresql also limits the jointable names at 64chars, but it doesn't give error so it seems postgre truncates the table name internally

devinivy commented 9 years ago

The solution to this primarily involves updates to waterline-schema so that it respects the new option. I'll take a deeper look at this today. Additionally, each adapter should deal with any constraints on their own. The postgresql adapter has the ability to shorten table names that are too long or to internally-to-the-adapter create its own junction table name convention. Or just throw an error and tell the user to use the soon-to-be feature of assigning a custom junction table name of appropriate length!

devinivy commented 9 years ago

I opened the requisite stories in sails-postgresql and waterline-schema to fully address this issue (see referenced issues above). The onus is no longer on waterline itself for now, so I'm closing this bug–it should be taken care of in the adapter.

dmarcelino commented 9 years ago

@devinivy, I got the impression from @mkeremguc that Postgres was handling the long names well:

it seems that postgresql also limits the jointable names at 64chars, but it doesn't give error so it seems postgre truncates the table name internally

@mkeremguc can you comment?

Anyway we also need a fix for MySQL adapter so I've raised balderdashy/sails-mysql#197.

devinivy commented 9 years ago

Cool, good thought! Do we need one for mongo too? Yeah, as I understand it, postgresql automatically does the truncation. Perhaps the adapters would want to deal with this more "smartly", esp for join tables. From http://www.postgresql.org/docs/9.3/static/sql-syntax-lexical.html ,

The system uses no more than NAMEDATALEN-1 bytes of an identifier; longer names can be written in commands, but they will be truncated. By default, NAMEDATALEN is 64 so the maximum identifier length is 63 bytes.

dmarcelino commented 9 years ago

Oh I see, you are probably right, the adapter can be made aware of it and at least throw an error if after truncation there is table name collision, for example.

Regarding mongo, I'll quote @nichdiekuh:

http://docs.mongodb.org/manual/reference/limits/

Namespace Length Each namespace, including database and collection name, must be shorter than 123 bytes.

It has a limit which is harder to hit... I would say MySQL is the priority.

mkeremguc commented 9 years ago

@dmarcelino, yea devinivy already proved postgre handles it internally. +1 for MySQL priority