Yeah and IMO this is critical enough to be backported. I guess not everyone uses inverse one to ones in projections 😆
In fact, I just used it because I was doing a synthetic test and it just so happened, that this kind of query was produced result in a strange test failure.

I amended the commit to use your wording. You can go ahead and merge it when you are ok with it.

@beikov you're insisting about it concerning inverse one to ones but don't you have the issue in any case if you mix scalar and entities for any type of relations?

Considering the fix, I would expect the issue to exist with any type of associations. And even with no associations (e.g. wouldn't something like select sum(a.myColumn), a from EntityA a).

AFAICT this only concerns inverse one-to-ones as these are special. Object of that type are constructed by their owners id rather than a dedicated key contained in the result set.

@beikov I rebased and force pushed.

I told you I found it a bit too dark magic for my taste and I broke your fix with a subsequent commit.

The issue is that you cannot consider that row will only contain values for your very specific case. It contains values for all the elements in SelectClause.fromElementsForLoad.

I think one idea would be to track more info in the SelectClause as we need to know which elements will be part of fromElementsForLoad (note that it includes the elements from the select clause but also potentially the ones of the fetch clauses). Either using a specific array or wrap the query return type in an object indicating if the element is a scalar or will be loaded.

Hmm, I'll have a look in the next days, but it looks like the includeInSelect array might have that information. I think we should always increment the managedTypeIndex when !includeInSelect[i], then this should work.

What I'm wondering is if we are not doing the work twice in the case of the first item of my updated test: it is present in row and we end up extracting it again from the result set.

I think that, if possible, we should exploit the elements coming from row.

@gsmet I think the fix is doing the right thing now.

Looks good, but initially I had the same worries as Guillaume. Left a couple of comments that might need some explanation.

Taking a step back here as I think the issue is larger than what Christian first suspected.

I think Christian got the biggest failure we can have with this issue: the expected element not being there at all but I think we make more work than we should in the other cases too.

There are two methods contributing to queryReturnTypes: initializeExplicitSelectClause() and initializeDerivedSelectClause().

Note that it's either one or the other. Think of both methods as different constructors of a SelectClause.

initializeExplicitSelectClause()

Standard select elements coming from

• the element is added to queryReturnTypes
• if the element is considered loadable, it is added to fromElementsForLoad
• it will be loaded and injected in the row array in this case

I don't think any existing array tells us that for a given queryReturnType, the element can be found in the row object. Thus I think we need one and I think we should always get the result from the row array to avoid reloading an instance that is already loaded (partially at this time but the objects will be hydrated anyway after that).

includeInSelect does not convey this: it is an array saying which elements from fromElementsForLoad should be included in the select clause.

Fetch elements coming from initializeExplicitSelectClause()

They will only be added to fromElementsForLoad after all our previous elements. They are not part of the return types so they can be ignored from our perspective.

initializeDerivedSelectClause()

We would need to understand when this is triggered to add tests.

In certain circumstances:

• elements are added to fromElementsForLoad
• some and only some of these elements are added to queryReturnTypes

Same as for the previous case, it would require a mapping of some sort.

Suggestion

I think I would vote for having a resultToLoadedElementMapping (better name very welcome) array to map the result to potential elements of row.

That would probably require building a List<Integer> in parallel of queryReturnTypes that we could flatten to an array of ints on finishInitialization(). -1 would mean that the element is not in the row array.

I would base all the logic in QueryLoader on this mapping so we wouldn't need to have any knowledge of the various cases in QueryLoader anymore and that would be nicely isolated. Typically @beikov I'm not sure your isEntityType() test is sufficient considering the logic in SelectClause#isReturnableEntity(). Maybe it is, maybe it isn't, we'll never know for sure so we should get the info from where it is created, not from another unrelated source.

We need to add more tests to cover the different cases.

HTH.

@jwgmeligmeyling looks like a good issue for you if you're interested (and my suggestions make sense to you).

I think we might be able to find a working solution this time and commit something :).

@gsmet I'd happily have another look at it, but I have limited availability for the next two weeks. I might get to it earlier, but probably not before then. Furthermore I might have another look at HHH-12842 first as it is a pretty nasty blocker for me personally (as I am limited to use not much earlier but certainly no newer than 5.2.12, which basically prevents migration to WF12 and WF13, WF12 because I can't get its Infinispan to work with Hibernate 5.2). I'd really like to get that out of the way and move to WF13 (or at that time 14) with Hibernate 5.3.

Also, I'd like to hear @beikov 's opinion regarding your findings as well. He seemed pretty adamant that his fix was sufficient.

About HHH-12842, I think you said Gail had a fix for it as part of other fixes. The patch will eventually get into 5.3. It's on the pending list for inclusion once Gail can finish working on the patch she initiated. Unfortunately, I doubt it will make it for WF 14, considering the feature freeze in on Wednesday.

Also, I'd like to hear @beikov 's opinion regarding your findings as well. He seemed pretty adamant that his fix was sufficient.

For now, I'm -1 on including the patch as is. It's too fragile as relying on an unrelated source of information to deal with the elements in row.

@beikov, I just created a PR for HHH-11903: #2600.

I believe HHH-11903 and HHH-12885 may be related. HHH-11903 only deals with a bidirectional association, where the non-mappedBy side is a derived ID. It may be possible to create a more general solution that fixes both HHH-11903 and HHH-12885.

@gbadner the issue seems unrelated to me. The problem is that this side of the one to one association is mapped by a OneToOneType, which has a fixed column span of 0 and resolves the entity based on the id of the owner. However, in QueryLoader, the owner is unknown and passed as null. We have to work around the nullSafeGet invocation on the OneToOneType.

After looking at the code in SelectClause, I see where you are trying to go @gsmet . I am however a bit cautious about the suggested change. The approach doesn't sound very intuitive to me and seems to suffer from the same issue: a poor communication contract between SelectClause and QueryLoader for scalar queries with entity projections. Getting this right may require more work than is appropriate under the scope of this issue.

Regarding your two concerns about the core assumptions from @beikov :

1. The only entity selections that are omitted from the fromElementsForLoad seem to be fetch joins. I already raised a question about how this behaves with questions with fetch joins. Subsequently @beikov added an additional test with a FETCH JOIN in 17e587a#diff-069946aa0f0181fcdefc3613a3a830a3R226 . Not sure if this covers all the cases yet.
2. It seems SelectClause#isReturnableEntity() returns true if the FromElement has an EntityPersister, so I think the isEntityType() should be safe.

The approach that @beikov suggests seem to work on the supplied test cases, and the implementation seems to be valid based on the current implementation of SelectClause. His test only affects Types with a column span of 0, which as far as I am aware of can only be inverse one to one relations. These changes leaves the normal execution path unchanged. The fix only affects the case that would be broken without the change (invalidly returning null). Therefore I think it should be safe to integrate this as is.

@jwgmeligmeyling your two points exactly summarizes my issue with this patch as it is (and tbh this remark is not limited to this work but concerns a lot of the code base):

1. seem to be Not sure if this covers all the cases yet.
2. It seems I think should be

I don't say Christian's reasoning is wrong or the patch in itself is wrong but it relies on assumptions, assumptions that could change with the life of the code.

That's why I wanted us to tie closely the creation of the information to the fact that we get it from there. I'm not sure I'll have the time to write some code before 5.4.0.CR1 though so if someone wants to try my suggestion and see how it goes, I don't think it's that much work.

Hi @gsmet , I understand your concern, and agree with your reasoning. Obviously I prefer a clean, proper solution too. My point was that even if the assumptions change, it would only break the case that is currently already broken (that is, as long as there are no other types besides inverse true one to ones that ever return a column span of 0).

I had started to implement your suggested approach prior to my previous commit, but I didnt get it to finish it yet.

@jwgmeligmeyling is it a large change? I didn't think it would be to be honest so I'm wondering if we're on the same page or if I missed something?

Maybe you could share your progress?

Thanks!

Is there anything I can do to make progress on this PR @gsmet ?

We just ran into this very same bug and thanks to @beikov we were lucky to identify the cause and work arround using Blaze Persistence EntityViews. Still i would love to see this one getting fixed - offering testing capacities :)