OpenJPA
  1. OpenJPA
  2. OPENJPA-2039

FKs for EAGER fields that are not in the current fetchplan aren't selected

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2.0
    • Fix Version/s: 2.2.0
    • Component/s: jdbc
    • Labels:
      None

      Description

      I am testing a scenario where I have an Entity with a number of eager/lazy relationships and at runtime, using a FetchPlan, I want to mark all relationships as lazy. I was able to hack around a bug reported previously on this list about not being able to remove fields from a fetch plan by creating a new fetch plan, removing the default fetch group, and re-adding all fields you want back into the new fetch plan. This all seems to work.

      I found that owned *toOne relationships that are marked as lazy via annotations we will select the foreign keys. If the relationships were marked as eager, we don't select the foreign keys. This is where I believe the bug is. In JDBCStoreManager.optSelect(...) we have a bit of code that looks to see if a field is a not a part of the default fetch group, and if it was not removed explicitly. This is wrong because for the sake of the load that is in progress, I'm not using the default fetch group.

      With this JIRA I'd like to remove the two conditionals '!fm.isInDefaultFetchGroup() && !fm.isDefaultFetchGroupExplicit()' so that we will load the fks for lazy and eager marked fields.

      This seems safe enough to do as worst case, we will select an extra field from a table that we are already selecting from. Best case, when accessing a lazy collection, we will issue a select by FK rather than a select with a join.

      1. OPENJPA-2039.failingtest.patch
        22 kB
        Rick Curtis
      2. OPENJPA-2039.patch
        10 kB
        Rick Curtis

        Activity

        Hide
        Albert Lee added a comment -

        Close issue in preparation for 2.2.0 release.

        Show
        Albert Lee added a comment - Close issue in preparation for 2.2.0 release.
        Hide
        Rick Curtis added a comment -

        Resolving issue.

        Show
        Rick Curtis added a comment - Resolving issue.
        Hide
        Rick Curtis added a comment -

        > a) change the doc comments on the interface to be more clear/elaborate in the user terms.

        That's where I'm stumbling. I really wouldn't expect most users to have the understanding of OpenJPA to want to control this aspect of loading. I only ran into this because of some of the trickery I'm trying to get working.

        If you get a chance, take a look at the unit test that I put together of this change.

        Show
        Rick Curtis added a comment - > a) change the doc comments on the interface to be more clear/elaborate in the user terms. That's where I'm stumbling. I really wouldn't expect most users to have the understanding of OpenJPA to want to control this aspect of loading. I only ran into this because of some of the trickery I'm trying to get working. If you get a chance, take a look at the unit test that I put together of this change.
        Hide
        Pinaki Poddar added a comment -

        Looks OK to me.
        Perhaps I would
        a) change the doc comments on the interface to be more clear/elaborate in the user terms.
        b) add an empty line between methods

        Show
        Pinaki Poddar added a comment - Looks OK to me. Perhaps I would a) change the doc comments on the interface to be more clear/elaborate in the user terms. b) add an empty line between methods
        Hide
        Rick Curtis added a comment -

        Reopening to address Pinaki's comments.

        Show
        Rick Curtis added a comment - Reopening to address Pinaki's comments.
        Hide
        Rick Curtis added a comment -

        > 1. ...The right (by existing design discipline) of any such configuration is not JDBCStoreManager, but JDBCFetchPlan/FetchConfiguration.

        Yes! I really struggled trying to figure out where to put this configuration. I never considered FetchConfiguration, I'll take a look at that shortly here.

        Show
        Rick Curtis added a comment - > 1. ...The right (by existing design discipline) of any such configuration is not JDBCStoreManager, but JDBCFetchPlan/FetchConfiguration. Yes! I really struggled trying to figure out where to put this configuration. I never considered FetchConfiguration, I'll take a look at that shortly here.
        Hide
        Pinaki Poddar added a comment -

        I have not tracked the details of this issue. But simply looking at the commit, I have few concerns.

        1. The new boolean flag introduced IgnoreFKSelect is not in the right place. The right (by existing
        design discipline) of any such configuration is not JDBCStoreManager, but JDBCFetchPlan/FetchConfiguration.
        I know it is a bit of extra work to carry that getter/setter, copying the state in FetchConfiguration.copy(),
        but I still request the committer to take that extra work to introduce this new option.

        2. A bit of JavaDoc comment will be helpful.

        3. A doc update is necessary.

        Show
        Pinaki Poddar added a comment - I have not tracked the details of this issue. But simply looking at the commit, I have few concerns. 1. The new boolean flag introduced IgnoreFKSelect is not in the right place. The right (by existing design discipline) of any such configuration is not JDBCStoreManager, but JDBCFetchPlan/FetchConfiguration. I know it is a bit of extra work to carry that getter/setter, copying the state in FetchConfiguration.copy(), but I still request the committer to take that extra work to introduce this new option. 2. A bit of JavaDoc comment will be helpful. 3. A doc update is necessary.
        Hide
        Rick Curtis added a comment -

        Committed revision 1157903 to trunk.

        Show
        Rick Curtis added a comment - Committed revision 1157903 to trunk.
        Hide
        Rick Curtis added a comment -

        Attaching another patch with the changes required to fix the one failing testcase.

        Show
        Rick Curtis added a comment - Attaching another patch with the changes required to fix the one failing testcase.
        Hide
        Rick Curtis added a comment -

        Attaching the code change and a testcase.

        Show
        Rick Curtis added a comment - Attaching the code change and a testcase.

          People

          • Assignee:
            Rick Curtis
            Reporter:
            Rick Curtis
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development