rsim / oracle-enhanced

Oracle enhaced adapter for ActiveRecord
MIT License
546 stars 308 forks source link

Override the max_index_name_size to keep local shortening algorithm. (8) #2373

Closed andynu closed 1 month ago

andynu commented 6 months ago

Rails now defaults to its own index name shortening (limited to 62 bytes). This change overrides the rails max to exceed this gems shortening method in order to preserve the historical behavior. names

See https://github.com/rails/rails/commit/3682aa1016e187398a7ae65e521f24f38b710d2b

So the shortening of index names that was added to rails itself was almost immediately changed to use an 'idx' prefix instead of an 'ix' prefix.

See https://github.com/rails/rails/commit/59f2b06493bed2c036f6b527e743198416ee1237

However, now that rails does shortening. It is an opportunity to consider simplifying this library and just using that.

Stacked on #2374

davinlagerroos commented 6 months ago

Something to consider: it appears oracle-enhanced has a practice of overriding these kinds of defaults. Following that pattern still gets spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:330 to pass and avoids the possibility of causing upgrade problems for someone who might have an existing schema that contains an index that is longer than 62 bytes.

andynu commented 6 months ago

I like this idea, but just overriding the max only works for newer oracle versions and isn't sufficient for oracle 11. https://github.com/rsim/oracle-enhanced/commit/7b36a5ce0b125523157dcf16c36e0d917cf42c98

davinlagerroos commented 6 months ago

Thanks for considering my suggestion! I think approach of aliasing the method still works with oracle 11. The method that would be aliased, max_identifier_length calls suports_long_identifier? which includes an oracle version check. For older versions of oracle suports_long_identifier? would return false and max_identifier_length would return 30.

It looks like in the test that breaks, the else branch of the test reflects this - when we are not testing with @oracle12cr2_or_higher the test's expectation is that the long index gets truncated to 30 characters.

andynu commented 6 months ago

@davinlagerroos I believe I tried what you suggest here https://github.com/rsim/oracle-enhanced/commit/7b36a5ce0b125523157dcf16c36e0d917cf42c98 But it didn't bear out in the tests: https://github.com/rsim/oracle-enhanced/actions/runs/8509850693/job/23306215927

I would welcome a review. Thanks!

davinlagerroos commented 6 months ago

@andynu My bad, I missed the test output! Thanks for making it more obvious. That is definitely not a good outcome.

hmm, when index_name calls super, activerecord uses the max_index_name_size to see if the name is too long and needs to be shortened. The change I suggested means that for oracle < 12.2, this is 30 and super returns a name shortened using activerecord's strategy.

However, oracle enhanced looks to be expecting to get the full activerecord index_name so it can shorten the name using its own strategy (where needed).

Would it work to define max_index_name_size as 128? Even though that is too long for oracle < 12.2, it should mean the call to super would return the full name and then the oracle enhanced could shorten it the way it wants to get to 30.

Or you can follow your original plan. That works too, it just might cause an issue if people have an index name that is longer than the new activerecord default. OTOH I do not know how many people would have an index name that long.

andynu commented 6 months ago

@davinlagerroos good idea! That has worked well and does a better job preserving existing behavior. I will rebase that into this PR tomorrow. https://github.com/andynu/oracle-enhanced/commit/696319271e666ff78eb6e46730bb8500509a80ff

davinlagerroos commented 6 months ago

@andynu Thanks for trying that out! I am glad it worked