Skip to content

Loading…

DDC-2989: ORM should allow custom index names for foreign associations. #3753

Closed
doctrinebot opened this Issue · 19 comments

2 participants

@doctrinebot

Jira issue originally created by user jsuggs:

Now that the DBAL allows/checks for renamed indexes, the ORM implementation needs to be enhanced to allow custom naming of the indexes for foreign associations.

@doctrinebot

Comment created by jsuggs:

Here is a full example
https://github.com/jsuggs/schema-issue

I think this mainly has to do with the doctrine auto-generated indexes.

@doctrinebot

Comment created by @ocramius:

IIRC, index names cannot currently be assigned, and are computed automatically - that's why the ORM is behaving like that.

I don't think this needs a fix right now - lowering priority

@doctrinebot

Comment created by @deeky666:

[~jsuggs] I also could not reproduce this bug in DBAL. I wonder if that is related to foreign key declarations (ORM relations) only. In fact, you cannot define index names for relations (foreign keys) in ORM at the moment. Therefore ORM always generates index names automatically. If you use custom names for those indexes in your online schema, the comparator will surely output the index renaming statement. I guess defining the index at table level in your mapping manually won't help here either.
Whatsoever I think this is an ORM only issue. If you manually changed your index names "online", you will now get some index rename statements when using the schema tool. Other users explicitly requested this feature. Schemas not in sync have to be upgraded, I guess. So IMO this is rather a "won't fix" in DBAL. However there is a possible improvement to ORM for being able to define index names for foreign keys. The whole topic is rather tricky concerning the comparator. Imagine you have (for whatever reason) multiple indexes with different names that span the exact same columns. How would you expect the comparator to output changes here? Which indexes should be renamed, which shouldn't?

@doctrinebot

Comment created by jsuggs:

I guess I get it, but its kinda a big deal and I'll have to old off upgrading as a result. I personally find this more of a regression due to the unintended consequences. The schema diff is now 100% useless (to me) as I rename all of my indexes to a more semantic name.

My off the cuff thought on ORM index naming is that you expand the naming strategy to include support for the indexes. Given the full set of criteria you should be able to come up with some sort of semantic naming and only on namespace collision to you "fallback" to a hash of the columns (solves your issues and mine).

@doctrinebot

Comment created by jsuggs:

To state differently, I (personally) value the schema diff tool much more than custom index (re)naming since the (overall) ability to use custom index names is not complete (due to the ORM issues Steve outlined above).

If the support to (re)name foreign key indexes can't go out in parallel to this, I'd like to offer a middle ground of making the index renaming configurable (ie ability to opt-out of functionality).

Sorry to be negative towards a new feature, but its really an inconvenience for me, and I suspect others since Strate also expressed concerns in the PR.

Let me know how I can be of help in working towards a resolution.

@doctrinebot

Comment created by @beberlei:

[~jsuggs] What do you mean with "just upgraded"? Which to which version? The index renaming and coverage feature is very old already, not sure what is happening. Do you define an index for a relation column?

@doctrinebot

Comment created by @deeky666:

[beberlei] This definitely has to do with the comparator now comparing index names also.
[
jsuggs] Can you please confirm my assumption that this problem is only related to relations and their foreign key indexes? Also can you confirm that it only affects those foreign key indexes which you gave a custom name? Can you please check that?
I don't see any reasonable solution in DBAL for this. The real solution IMO would be to allow custom names in ORM mappings for foreign key realation indexes.

@doctrinebot

Comment created by jsuggs:

Sorry for the confusion.

Here is the PR that introduced the functionality in question.
doctrine/dbal#473

Benjamin, I had just upgraded DBAL to the latest 2.5 master. This is only indirectly related to the ORM functionality as the ORM SchemaTool uses the DBAL Comparator to generate is schema diff.
Steve, yes this is only related to relations and foreign key indexes that were given a custom name. I documented the use case/scenario here: https://github.com/jsuggs/schema-issue

Steve, I see three potential solutions (in my personal order of preference).
1) Expand the NamingStrategy to allow it to handle the default names of foreign key relation indexes
2) As you suggested, allow for ORM mappings to specify foreign key relation indexes
3) Add a configuration directive that essentially allows for you to ignore renamed indexes (ie. fallback to the previous behavior, prior to PR 473).

I realize that #1 would be a bit more work, but I think offers enhanced functionality. #2 is a good, but would require additional annotations/mappings. #3 is probably the least dev effort but just doesn't feel right (to me).

Again, I don't want to seem critical of the dev effort, but its an unintended consequence that will keep me from being able to upgrade DBAL to 2.5.

@doctrinebot

Comment created by @deeky666:

[~jsuggs] You are right the comparator is somewhat responsible for the "unnecesarry" schema diffs it now creates. However its functionality is completely working IMO. Try creating and comparing your schema example on DBAL level only (without utilizing ORM). You will see that it works. I see and understand that those index name updates are annoying but they are not WRONG. If you have a mapping for a relation with an unnamed index and rename this index in your online schema manually, I would even expect the schema tool to generate this diff. Because this is the whole point of the index rename feature. The problem we have here is that there currently is no way in ORM to define the index name on association mappings. There the following happens in your example when comparing the online and offline schema with the schema tool:

(abbreviated to the important steps, chronological order)

  1. Collect mapping information and evaluate offline schema
    1.1. Schema tool collects relation SQL for Bar -> Foo association and adds an unnamed index "IDX_76FF8CAA8E48560F" to table "bar"
    1.2. Schema tool collects custom indexes defined in the mapping for "bar", detects the custom index "IDXMANY_TO_ONE", tries to add it to the table "bar, but discards it because there is already another index "IDX76FF8CAA8E48560F" fulfilling the exact same columns (this is how DBAL works ever since to avoid duplicate indexes)

  2. Reverse-engineer online schema from database
    2.1 Fetch all defined indexes for table "bar" -> adds "idxmany_toone" to table "bar"

  3. Comapre evaluated online schema to evaluated offline schema
    3.1 Compare index "idxmany_to_one" (online schema) to index "IDX_76FF8CAA8E48560F" (offline schema) -> suggest index rename from "idx_many_to_one" to "IDX76FF8CAA8E48560F" because the mapping is your definition and takes precedence.

Just to clarify what happens under the hood and that it is an ORM issue, not DBAL!

@doctrinebot

Comment created by jsuggs:

Here is a first shot at basic support for allowing the mapping of the index name.
https://github.com/jsuggs/doctrine2/compare/foreign-association-index-names

@doctrinebot

Comment created by @deeky666:

[jsuggs] I have worked on a PR for this already using the exact same approach you just mentioned. However I am not quite sure about ManyToMany yet (because you would have to be able to define both index names there) and also I talked to [beberlei] and he does not seem to be happy with this approach. So I stopped work on this for the moment.

@doctrinebot

Comment created by @deeky666:

Moved this to ORM issues.

@doctrinebot

Comment created by jsuggs:

Steve I both agree and disagree.

I think my underlying issue is your step 1.1. The ORM creates the index named "IDX76FF8CAA8E48560F" and there is NO extension point for being able to override that behavior. FWIW, I've never been fond of the way that the ORM uses the AbstractAsset::generateIdentifierName for generating the index and foreign key.

So I definitely agree that its is an ORM issue not DBAL issue :). However, as I stated previously, until the ORM is updated/patched to allow for index naming, this (in my opinion) is a regression despite it working the way it currently does (correct or not).

I can close out this issue and open one for ORM. Do you have a old branch and/or ORM ticket that I can reference?

Thanks for engaging in the dialog! I hope to be of assistance in helping come up with a solution that gets everyone happy.

@doctrinebot

Comment created by @deeky666:

[~jsuggs] Thanks for updating the description and thanks for assisting on this issue. We will find a solution for this before the final release of 2.5. If we cannot find a good solution until then we might also consider reverting the index renaming feature on DBAL (but I would like to avoid that). I will discuss this issue with the other core devs again and see what we can come up with.

@doctrinebot

Comment created by jsuggs:

No problem, here is the start of a PR to maybe address the index names
#956

@doctrinebot

Comment created by @doctrinebot:

A related Github Pull-Request [GH-956] was assigned:
#956

@doctrinebot

Comment created by @doctrinebot:

A related Github Pull-Request [GH-956] was closed:
#956

@doctrinebot

Issue was closed with resolution "Fixed"

@Ocramius Ocramius was assigned by doctrinebot
@doctrinebot doctrinebot added this to the 2.5 milestone
@doctrinebot doctrinebot closed this
@doctrinebot doctrinebot added the Bug label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.