oasp / oasp4j

The Open Application Standard Platform for Java
Apache License 2.0
60 stars 303 forks source link

moved pagination TOs to basic module as they are not JPA specific #630

Closed maybeec closed 6 years ago

maybeec commented 6 years ago

As of support for different persistence technologies we observed, that pagination support was wrongly put into the jpa module although it is not jpa specific. This PR resolves this failure by moving the pagination TOs to the basic module.

hohwille commented 6 years ago

We should IMHO align this with #627 as that PR is already introducing new TOs in the jpa modules. These do not need to be introduced as deprecated copies. If all agree, I would change my fork/branch and update PR #627 as plannend. Then we can close this PR. WDYT?

maybeec commented 6 years ago

So #627 will go for oasp4j 3.0 alpha already without introducing deprecated copies? I am fine with that. However, should we introduce this solution also in oasp4j 2.x? If so, we should create another PR with the deprected copies as well? Maybe I can even change this PR. The question here is, whether we need this movement already in 2.x. Are there any needs for that?

@ivanderk do we want to have mybatis and spring data already be available in 2.x? If so, we may even want to adapt #627 to be backward compatible.

hohwille commented 6 years ago

627 neither moves nor copies nor deprecates any of these classes. As I stated it introduces new classes in the "old" location and was already raising the question that the "old" location is odd/wrong.

IMHO we should:

IMHO we can merge this PR already for a 2.x branch and then merge to 3.0 while PR #627 should only go into 3.0.

maybeec commented 6 years ago

old classes recovered and deprecated

hohwille commented 6 years ago

Thanks for your update. However, when I am talking about compatibility and deprecation I would expect that old code still works without compile or runtime errors. Therefore we also need to keep the old methods that work with these objects but mark them as deprecated as e.g.: https://github.com/oasp/oasp4j/blob/6c6c9cb7c503af79ee324c9aad2382e41137dfd8/samples/core/src/main/java/io/oasp/gastronomy/restaurant/general/logic/base/AbstractComponentFacade.java Therefore also note #539 And also: https://github.com/oasp/oasp4j/blob/6c6c9cb7c503af79ee324c9aad2382e41137dfd8/modules/jpa/src/main/java/io/oasp/module/jpa/dataaccess/base/AbstractGenericDao.java#L252

BTW: Is this a bug of github that in the Files changed tab of this PR it shows classes like OrderByTo as added to basic and removed from jpa even though your last commit has changed this?

maybeec commented 6 years ago

I am not pretty sure, what you are talking about. Lets see

BTW: Is this a bug of github that in the Files changed tab of this PR it shows classes like OrderByTo as added to basic and removed from jpa even though your last commit has changed this?

on top, you can click "view all", which gives you the summary of all commits.

Thanks for your update. However, when I am talking about compatibility and deprecation I would expect that old code still works without compile or runtime errors.

Why should the new code run into compile time errors? You mean, if you use a new version of jpa and and old of basic module? If so, do you really want to support it? Than you have to duplicate also the dependent classes like AbstractGenericDao to once use the new impl of basic module and once use the old impl of jpa module. May be I even did not get your point.

hohwille commented 6 years ago

BTW: OrderDirection is not a transfer object (TO) but we always locate stuff like this in to sub-package. It is actually a data-type (aka value-type). Maybe we should also rather not separate this stuff into to and datatype but rather into something like query and/or paging. WDYT?

maybeec commented 6 years ago

Query vs to is kind of a hen-egg-problem. We are using the datatypes and abstract TOs in the service layer as well as in the database layer to construct queries. For simplicity and understandability reasons, I would keep to, i.e. with SearchCriteriaTo, and move datatypes to datatype package. If you like you can even have another sub package in datataype, which indicates paging.

hohwille commented 6 years ago

on top, you can click "view all", which gives you the summary of all commits.

That is exactly what I did. Please go there and search for OrderByTo.java. You will get one add and one remove. The remove should however be an update with the deprecation. Am I still missing something here?

Why should the new code run into compile time errors?

Simply because you changed code in an incompatible way. I also included 2 links above. First one was: https://github.com/oasp/oasp4j/blob/6c6c9cb7c503af79ee324c9aad2382e41137dfd8/samples/core/src/main/java/io/oasp/gastronomy/restaurant/general/logic/base/AbstractComponentFacade.java This is pointing to the jpa classes in 2.5.0 and will now point to the basic classes in 3.0.0.

However, before we proceed here we should have a discussion with the community. I see the following issues here:

  1. We are entirely reinventing the wheel here: https://github.com/hohwille/oasp4j/wiki/paging
  2. We will break compatibility anyhow with 3.0.0 - see https://github.com/oasp/oasp4j/issues/614. Therefore my argument to keep compatibility or put energy into that may be void.
  3. The impacted support classes would be quite messy if we keep the old deprecated methods and add new ones with almost same signatures. Further there are methods that seem to offer support but in their implementation they only throw a UnsupportedOperationException what is sick.

So let us better first agree on a general strategy to improve. Next we can think about compatibility as a secondary task. But for 3.0.0 we might dare to do some tidy-up that may imply small upgrade effort. Still there is also the naming issue that might even go hard on compatibility anyhow.

amarinso commented 6 years ago

For compatibility issues... Could we move all deprecated classes to a new maven module so when upgrading the project only has to add oasp4j-legacy/pom.xml until the project has the chance to change it step by step?

I dislike both having everything failing when upgrading and having legacy code on the master branch. Maybe we could even give hints on the console when building/running about legacy code being used and tips to migrate...

Just an idea...

maybeec commented 6 years ago

You get the deprecation warnings by maven anyhow if you use deprectated code in a build. Therefore, I would vote for breaking on upgrade and provide a upgrade path documented in the changelog.

maybeec commented 6 years ago

This is pointing to the jpa classes in 2.5.0 and will now point to the basic classes in 3.0.0.

now I got it... I am loosing track of the common development branch and all this release branches, which may have a different version as you see on the shared branch as there are so many PRs open in parallel, where you even don't know whether it will be merged into 2.5. or 3.0... Hope we can merge some of them soon.

hohwille commented 6 years ago

@amarinso

For compatibility issues... Could we move all deprecated classes to a new maven module so when upgrading the project only has to add oasp4j-legacy/pom.xml until the project has the chance to change it step by step?

I guess after our vote for using spring-data stuff this PR is actually outdated. I did more or less exactly what you suggested in this PR #627. See e.g. https://github.com/hohwille/oasp4j/blob/9325b6d662f567d6be27ffd745777d170673704b/modules/jpa/src/main/java/io/oasp/module/jpa/common/api/to/OrderByTo.java#L8 However, for DAOs the old methods for paging have been removed and instead I added this legacy class: https://github.com/hohwille/oasp4j/blob/9325b6d662f567d6be27ffd745777d170673704b/modules/jpa/src/main/java/io/oasp/module/jpa/common/base/LegacyDaoQuerySupport.java#L23

This allows projects to easily support the "old paging stuff" while keeping the oasp codebase clean and be able to easily get rid of deprecated/old stuff. I will close this PR (thanks @maybeec for it and sorry for closing). If you have feedback about the legacy support or compatibility please comment PR #627