Skip to content

Fix parameter detection for custom hql method generators #2793

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

Merged
merged 3 commits into from
Jun 10, 2021

Conversation

maca88
Copy link
Contributor

@maca88 maca88 commented May 30, 2021

Fix for a regression made by #2365.

Fixes #2791

@bahusoid
Copy link
Member

What do you think of #2723?

I believe this change is not required on master if #2723 is merged. Do you see any drawbacks? All tests are green except the one I added (and most likely it fails on dialects that don't distinguish/support AnsiString type)

@maca88
Copy link
Contributor Author

maca88 commented May 31, 2021

What do you think of #2723?

I think that your approach is better as we preserve the guessed type in case we would need it in the future. So probably the best would be to rebase your PR to 5.3.x and close this one.

All tests are green except the one I added (and most likely it fails on dialects that don't distinguish/support AnsiString type)

The StartsWith/Contains/IndexOf/... tests can be fixed by adding their method definition to this line:
https://github.com/nhibernate/nhibernate-core/blob/5.3.8/src/NHibernate/Linq/Visitors/ParameterTypeLocator.cs#L249

maybe we need to make this line configurable by extending IHqlGeneratorForMethod.

@bahusoid
Copy link
Member

bahusoid commented Jun 1, 2021

So probably the best would be to rebase your PR to 5.3.x and close this one.

I'm not sure. IMO it's too general change for patch release - as you can see I had to adjust few places to make it work (like in StringGenerator and CaseNode classes) . So other unnoticed regressions are also possible.

@maca88
Copy link
Contributor Author

maca88 commented Jun 1, 2021

I understand, so a safer option would be to merge this one and remove its changes in your PR.

@PleasantD
Copy link
Contributor

The StartsWith and Contains tests can be fixed by adding their method definition to this line:
https://github.com/nhibernate/nhibernate-core/blob/5.3.8/src/NHibernate/Linq/Visitors/ParameterTypeLocator.cs#L249

maybe we need to make this line configurable by extending IHqlGeneratorForMethod.

I agree that exposing the list of methods that are eligible for this relationship finding is desirable.
Since generator logic is customizable, it makes sense to me that this could be needed by arbitrary user defined methods.

Otherwise, this seems a minimally adequate solution for issue #2792

@fredericDelaporte fredericDelaporte added this to the 5.3.9 milestone Jun 6, 2021
@bahusoid

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants