Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong statement when child classes have same property name but different types #3363

Open
FelicePollano opened this issue Jul 14, 2023 · 11 comments

Comments

@FelicePollano
Copy link

FelicePollano commented Jul 14, 2023

I think there is something not working while generating a query in a hierarchy of classes in which some derived classes have an association property with the same name but different many-to-one class. ie:

   				+----------+
   				+  mother  +
   				+----------+
   			 
   			 
   +-------------+				+-------------+
   +    Child1   +				+    Child1   +
   ===============				===============
   | Thing:Thing1|				| Thing:Thing2|
   +-------------+				+-------------+

So if we look for a specific value of Thing property like this:

var result = session.Query<Mother>().Where(k => k is Child1 && (k as Child1).Thing.Id == "00001").ToList();

We do not find any record even if the record exists, as the query generated is left-joining the wrong table, ie:

select mother0_.Id as id1_0_, mother0_.Name as name3_0_,
		mother0_.thingId as thingid4_0_, mother0_.kind as kind2_0_ from Mother mother0_ 
	left outer join Thing2 thing2x1_ on 
		mother0_.thingId=thing2x1_.Id where mother0_.kind='1' and thing2x1_.Id=?"

That is, looks into table for Thing2, while we actually look for a Thing1...

Unit test in PR #3364

FelicePollano added a commit to FelicePollano/nhibernate-core that referenced this issue Jul 14, 2023
@bahusoid
Copy link
Member

Looks like duplicate of #1189

@FelicePollano
Copy link
Author

FelicePollano commented Jul 14, 2023

Actually can be same issue. I can add I had the case working in production until 5.2.7, it breaks after 5.3.0 included.

@fredericDelaporte
Copy link
Member

So, it would be a 5.3.x regression. I consider checking this before releasing the next 5.4.x.

@hazzik hazzik changed the title Wrong statement generated when child classes have same propery name but different type. Wrong statement generated when child classes have same propery name but different type Jul 18, 2023
@bahusoid
Copy link
Member

I had the case working in production until 5.2.7, it breaks after 5.3.0

I see it's about #1189 + not-found="ignore" mapping. In 5.2.7 join wasn't generated for (k as Child1).Thing.Id == "00001" check. But in 5.3 LEFT JOIN is generated to make sure that not found records are ignored in query too. And this gives us join on ambiguous name - #1189

I have no good fix for this case in mind - you better avoid using the same names in subclasses for different entities.

@FelicePollano
Copy link
Author

FelicePollano commented Jul 18, 2023

I have no good fix for this case in mind - you better avoid using the same names in subclasses for different entities.

actually this is current solution adopted, but, if this is the case, some exception in mapping phase should complain, otherwise is easy to be confused.

FelicePollano added a commit to FelicePollano/nhibernate-core that referenced this issue Jul 18, 2023
@fredericDelaporte fredericDelaporte changed the title Wrong statement generated when child classes have same propery name but different type Wrong statement when child classes have same property name but different types Jul 18, 2023
@FelicePollano
Copy link
Author

By looking at current code base, looks like there is something wrong here: ( AbstractPropertyMapping )

protected void AddPropertyPath(string path, IType type, string[] columns, string[] formulaTemplates)
{
			typesByPropertyPath[path] = type;
			columnsByPropertyPath[path] = columns;

			if (formulaTemplates != null)
				formulaTemplatesByPropertyPath[path] = formulaTemplates;
}

So this function is called collecting a property name/type map for the entire hierarchy of an entity, so, if down the entire tree some property have same name, entry will be overwritten, causing unexpected behaviors. I had issues compiling 5.2.7 tag because of missing requirement, but maybe in the previous version this was called only for single entity?

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Jul 23, 2023

No, I do not think this has changed. What has changed is #2081, fixing #1274.

Your mapping uses not-found="ignore" on the relation, which means a not null foreign key does not guarantee the associated entity exists. Prior to #2081, in case of comparison on the associated ID, NHibernate would do that on the foreign key without checking the associated entity actually exist, which could yield wrong results in case the entity was not actually existing.
Now it checks the associated entity existence, but is hit by #1189 in case of polymorphism.

(About issues for compiling, switching from 5.4.x to a lower version branch usually requires cleaning, closing the solution in VS, restoring. Actually, it is more reliable to close the solution after switching branch, purge all bin and obj folders, reopen the solution.)

So, by fixing a not-found="ignore" bug in some cases, #2081 has caused another bug (#1189) to be extended to some not-found="ignore" specific cases. By the way, your application was surely hit by the bug fixed by #2081, unless it does not actually have missing entities. If it does not have missing entities, remove not-found="ignore". It will solve the trouble.

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Jul 23, 2023

So, this is still a regression caused by #2081. But maybe a tough one to fix, hitting what is in my opinion a corner case (not-found="ignore" + polymorphism with different association properties sharing the same name). Not sure we should mandate a fix for this one in 5.3.x.

@bahusoid
Copy link
Member

bahusoid commented Jul 24, 2023

Not sure we should mandate a fix for this one in 5.3.x.

Agree. Not easy fixable. No need to wait

I see that in the test case the same column name thingid is used to reference two different tables (Thing2 and Thing1). If not-found="ignore" is added just to avoid foreign key creation - you should use foreing-key="none" mapping instead.

@FelicePollano
Copy link
Author

FelicePollano commented Jul 25, 2023

Hi @bahusoid, no, it is not to avoid foreign key creation. By the way, even if not modified, from what I saw, the function AddPropertyPath mentioned above, looks like it is the source of the problem. In any hierarchy, if two properties share same name, and that could actually happen for any reason, the type map is overwritten by the last entry found. Here the problem. I suspect that can be causing some issues somewhere else as well. Short "fix", according to me, could be at least throw an exception when this happens, pointing to the undesired duplicated property name.

@fredericDelaporte
Copy link
Member

Yes the issue is more general (and known since long), but complex to fix, see #1189.

Still, your specific case could be fixed by a "work-around": cease using that other "work-around" that is not-found="ignore".

(That ignore feature is a work-around for supporting incoherent data in the database. It is best to not use it if possible. This means, fix invalid data and enforce in the DB foreign key constraints to avoid new invalid data, then remove from the mapping these ignore settings.)

hazzik pushed a commit to FelicePollano/nhibernate-core that referenced this issue Jun 4, 2024
hazzik pushed a commit to FelicePollano/nhibernate-core that referenced this issue Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants