Query clone support with pre-existing entityManager/connection #672

[TuomasKiviaho]

No description provided.

[timowest]

This looks really good. The only comment I can come up with is that JPA specific SQL subquery classes are not necessary. SubQuery classes are only builders for sub query expressions. The serialization logic and execution bindings (EntityManager or Session) are in the main query class.

[TuomasKiviaho]

It kind of slipped out prematurely.

Currently there is neither support for native SQL based DMLClauses nor SubQueries which both combined would make it possible to provide JPASQLQueryFactory that would hide the need for entityManager referencing. But that's another story for another time and I removed it for the time being.

[timowest]

I removed the JPA specific SQLSubQuery which didn't compile.

[timowest]

@TuomasKiviaho There are still some compilation issues on the querydsl-jpa test side. Could you look at them?

[ERROR] /home/tiwe/work/querydsl/querydsl-jpa/src/test/java/com/mysema/query/jpa/JPAIntegrationBase.java:[55,49] cannot find symbol
  symbol: method getConstants()
[ERROR] /home/tiwe/work/querydsl/querydsl-jpa/src/test/java/com/mysema/query/jpa/HibernateQueryTest.java:[35,32] reference to clone is ambiguous, both method clone(org.hibernate.Session) in com.mysema.query.jpa.hibernate.AbstractHibernateQuery and method clone(org.hibernate.StatelessSession) in com.mysema.query.jpa.hibernate.AbstractHibernateQuery match
[ERROR] /home/tiwe/work/querydsl/querydsl-jpa/src/test/java/com/mysema/query/jpa/IntegrationBase.java:[53,55] cannot find symbol
  symbol: method getConstants()
[INFO] 3 errors 

[timowest]

Sorry that I merged already. Next time I will do a more thorough review.

[timowest]

@TuomasKiviaho Did you test whether the changes related to moving methods upwards in the class hierarchy are binary backwards compatible?

[timowest]

I fixed now the compilation issues, but there are still test failures related to native query syntax differences, could you take a look when you have time?

[timowest]

Ok, now it should be stable again. I think the next time we need to split these changes into smaller more manageable steps.

[TuomasKiviaho]

Sorry about the wast amount of unnecessary refactorings. This is what happens when you suddenly have too much free time at your disposal. The serializer fix was a good catch from you. I hope you didn't waste too much time figuring it out.

https://github.com/jeluard/semantic-versioning might be handy to ensure that backwards compatibility is preserved.

I seem to have managed to use -Dmaven.test.skip=true instead of -DskipTests for the JPA so basically forgot to test it completely. Jenkins has a github pull request plugin that might be handy so that a pull request could be validated prior merging. CouldBees seems to be providing it as well (unlike M2 release plugin) so that there would not be a need to expose your current CI.

[timowest]

Thanks, I will look into semantic-versioning and the pull request validation options.

[Shredder121]

Why don't you fetch the branch and review it that way?
or how do you review code? @timowest

maybe you can also ask others for their opinion if you are in doubt?

[timowest]

Why don't you fetch the branch and review it that way?

Yes, that should be definitely part of the protocol, but also automated validation of pull request content would be nice. I plan to move all major Querydsl changes to pull requests.

I am a bit new to pull request usage, so suggestions are welcome.

[Shredder121]

Probably automated build servers can verify the compiling and testing of the build, but manual reviewing is still preferred/mandatory if you ask me.

Also, just asking. can you tell GitHub to merge the pull request in another branch from your perspective? That way you could set up a sort of git flow, with proposed-[issue], hotfix-[issue] and merging them into validated/test, then merging to master.
that way, if you accidentally merge(or overlook something), you don't merge in master, so the branch heads differ, and you don't have to pull the branches apart.

[timowest]

Also, just asking. can you tell GitHub to merge the pull request in another branch from your perspective?

I don't know, that would be a good option.

[Shredder121]

What you can do, is making an acceptance branch and manually merge that, tracking the pull request branch.
then you can review the changes, push commits (to the pull request branch, since others can merge that as well) and when you're done you can easily accept the pull request in master.
I don't know how easily you can adopt it or how feasible you think it is, but it is just a suggestion.
also, you can't relocate github pull requests, unfortunately.

[timowest]

I added enforcer (semantic versioning) via a pull request #717

There are some incompatibilities, I will try to find a solution. The incompatibilities are on the binary level mostly, but for other frameworks that use Querydsl such as Spring Data this is quite critical.

I will try to come up with a solution.