Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

NH-3303: Refactoring of custom SQL code #161

Closed
wants to merge 9 commits into from

3 participants

@ggeurts

Major refactoring of custom SQL code to reduce code duplication and simplify maintenance and bug fixes.

The following classes have been affected mostly:

  • NHibernate.Loader.Custom.Sql.SqlQueryReturnProcessor

    This class had 2 responsibilities. 1) It provided a lookup mechanism for persisters, suffixes and user-defined property result maps, based on raw information read from INativeSQLQueryReturn instances. 2) It transformed INativeSQLQueryReturn instances into NHibernate.Loader.Custom.IReturn instances for use by the NHibernate.Loader.Custom.CustomLoader.

    The code related to the first responsibility has been moved into the new NHibernate.Loader.Custom.Sql.SqlQueryContext class. The code related to the second responsibility has been integrated into the NHibernate.Loader.Custom.Sql.SqlCustomQuery class.

  • NHibernate.Loader.Custom.NonScalarReturn

    The return transformation code that originally resided in NHibernate.Loader.Custom.Sql.SqlQueryReturnProcessor and now is part of NHibernate.Loader.Custom.Sql.SqlCustomQuery, was greatly simplified by removing all IReturn implementations that extend NonScalarResult and make the NonScalarResult class a bit smarter.

  • NHibernate.Loader.Custom.CustomQuery

    Removed code that adapted lookup functionality of NHibernate.Loader.Custom.Sql.SqlQueryReturnProcessor for use by NHibernate.Loader.Custom.Sql.SqlQueryParser class. This functionality is now directly provided by the new NHibernate.Loader.Custom.Sql.SqlQueryContext class.

    Contains simplified code to transform INativeSQLQueryReturn instances into NHibernate.Loader.Custom.IReturn instances. This transformation code originates from the now defunct NHibernate.Loader.Custom.Sql.SqlQueryReturnProcessor class.

  • NHibernate.Loader.Custom.CustomLoader

    The constructor code has been simplified a lot because of the reduced number of IReturn implementations.

@hazzik
Owner

Hi, @ggeurts. Could you remove merge commit e6bbb04 from the pull request?

@ggeurts

The merge commit has been removed.

@oskarb
Owner

Hibernate seems to have done some serious Loader refactoring work in 2012-2014. Haven't looked at the details, but I worry that some of these changes might complicate porting. Any ideas on this?

@oskarb

This seems to cause test failures. Not sure what to do about them.

@oskarb
Owner

I've merged the first two commits (cc46650 and 2bd8bc7) and cherry-picked 71a4165 to master. The intervening 69cd430 causes test failures (at least on current master) , that I couldn't find any resolution for in the other commits (plus I wonder if those failures indicate this might be a breaking change for some users).

If more work on this is done, please rebase the remaining commits on current master. However, a comparison with the current state of the corresponding Hibernate classes should be made, so we don't divert from the structure too much without a very clear benefit.

@oskarb oskarb closed this
@hazzik
Owner

@oskarb can we avoid such long pull requests? I'll prefer to apply a patch from a pull request, or rebase on top of master.

@oskarb
Owner

I assume that you mean that I merged commits that were two years old? Well I don't feel strongly about it. The benefit I thought was to avoid "duplicated" commits, i.e. git now knows that two commits from this PR was merged. Obviously it should not be done if the merge commit itself involves complicated conflict resolution. What drawbacks do you see?

@ggeurts ggeurts deleted the branch
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.