This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 223317 - Profile should have method for better version comparison
Summary: Profile should have method for better version comparison
Status: RESOLVED FIXED
Alias: None
Product: javaee
Classification: Unclassified
Component: Code (show other bugs)
Version: 7.3
Hardware: PC Linux
: P3 normal (vote)
Assignee: Martin Janicek
URL:
Keywords: API, API_REVIEW_FAST
Depends on:
Blocks: 223442
  Show dependency tree
 
Reported: 2012-12-05 13:50 UTC by Martin Janicek
Modified: 2013-02-13 01:51 UTC (History)
7 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
Patch implementing Profile.isEqualOrHigher (5.24 KB, patch)
2012-12-05 14:00 UTC, Martin Janicek
Details | Diff
Version one implementation (6.28 KB, patch)
2012-12-06 10:34 UTC, Martin Janicek
Details | Diff
Version two implementation (3.50 KB, patch)
2012-12-06 10:35 UTC, Martin Janicek
Details | Diff
Proposed solution (6.83 KB, patch)
2012-12-06 13:56 UTC, Martin Janicek
Details | Diff
MF02 - @NonNull annotations added (6.96 KB, patch)
2012-12-06 14:41 UTC, Martin Janicek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Janicek 2012-12-05 13:50:22 UTC
Quite a lot of our Java EE code works differently based on the Java EE profile defined for a certain project and thus we often need to find out if the current profile version supports features defined in specific Java EE version. Also typically newer version of Java EE contains the same functionality as the one before + some additional features. 

Currently we don't have an easy way how to find out if the profile is for example J2EE 1.4 and higher. That leads to a lot of conditions in our code-base where we can see something like this:

if (Profile.JAVA_EE_6_WEB.equals(profile) || Profile.JAVA_EE_6_FULL.equals(profile)) ...

With upcoming Java EE 7 where two new profiles were added (JAVA_EE_7_WEB and JAVA_EE_7_FULL) this would became even scarier as we would need to go through those conditions and in most cases change them to:

(Profile.JAVA_EE_6_WEB.equals(profile) || Profile.JAVA_EE_6_FULL.equals(profile) || Profile.JAVA_EE_7_WEB.equals(profile) || Profile.JAVA_EE_7_FULL.equals(profile))

I would like to introduce a new method Profile.isEqualOrHigher(Profile profile) which does exactly the same thing. It compares current ("this") profile with the given one and if it's the same version or higher it will return true, false otherwise. Using this method we could avoid of changing the conditions with every new profile as described above.


One more note: Maybe we could consider also adding Profile.isEqualOrLower(Profile profile) but I don't see real use case at the moment (but I have been looking only in maven.j2ee module)
Comment 1 Martin Janicek 2012-12-05 14:00:29 UTC
Created attachment 128896 [details]
Patch implementing Profile.isEqualOrHigher
Comment 2 Martin Janicek 2012-12-05 14:21:18 UTC
Please review.
Comment 3 Petr Hejl 2012-12-05 14:50:45 UTC
[PH01] I think this basically introduce Profile as Comparable. So perhaps it would be better to have it this way (implementing Comparable). However in that case it is not clear what is particular order for example for Full and Web.
[PH02] Actually the code usually does not care about the profile, but about functionality it offers (or not). So with this method the code will still be polluted by something like Profile.JAVA_EE_5.isEqualOrHigher(profile).
[PH03] There has been similar effort when introducing Java EE 6. See J2eeProjectCapabilities in j2ee.common. This would be better place imo rather than putting such thing directly to public API.
[PH04] J2eeProjectCapabilities.isSomethingSupported..() would be imo better solution in long term. If you feel you desperately need the clean out now I think a method J2eeProjectCapabilities.isAtLeastJavaEE5() or something similar would be better than Profile.JAVA_EE_5.isEqualOrHigher(profile).

Don't get me wrong I appreciate the idea and it is good to know there are people who care about the code ;)
Comment 4 Petr Hejl 2012-12-05 14:59:46 UTC
Just for the record - the right solution (not really something to do at the moment) would be use of javaee.specs.support. At least for new things.
Comment 5 Martin Fousek 2012-12-05 15:14:58 UTC
Although I don't feel that it's proper to call i.e. JAVA_EE_7_WEB as "higher" than JAVA_EE_6_FULL but the real look into our codes speaks that having something like that is good initiative now.

[MF01] Profile.JAVA_EE_6_FULL.isEqualOrHigher(Profile.JAVA_EE_6_WEB) looks to me unclear since it's not apparent how it's compared. I would consider something like PH04/PH01 or static method for Profiles comparsion.

[MF02] There are missing in the API Common Annotations

[MF03] PH03 +1 :)
Comment 6 Sergey Petrov 2012-12-06 06:48:06 UTC
[SP1]  even if implement something, may be wording and logic should be changed a bit isVersionHigherOrEqual() as said in [MF1]  what should mean and what should return JAVA_EE7_WEB.isHigherOrEqual(JAVA_EE6_FULL) may not be clear.
Comment 7 Denis Anisimov 2012-12-06 07:39:54 UTC
I agree with Petr and Martin.
Profiles are not comparable by the nature and there is no need in such
comparing. It is required to know that chosen profile has at least features
declaring by other profile in most usecases .
So I would suggest to move suggested "comparing" out of the Profile class.

And I agree that such functionality could be useful. We overuse Profile in 
mentioned conditions like:
if (Profile.JAVA_EE_6_WEB.equals(profile) ||
Profile.JAVA_EE_6_FULL.equals(profile))
Comment 8 Martin Janicek 2012-12-06 10:33:32 UTC
Ok, agree that it make sense to add the comparison logic to the J2eeProjectCapabilities instead of Profile class (which is PH03, PH04, MF03 I guess:)).

Few ways how we could do this:

1) One method isVersionEqualOrHigher(..) with two Profile parameters (the first one is the compared one and the second one we are comparing against)

2) Single method for each use case (currently I see the need only for isAtLeastJavaEE5 and isAtLeastJavaEE6, but I'm working only with a small subset of the Java EE modules so maybe there are some others)

3) One method with two parameters (Profile class and some "strategy" one used for comparison - with predefined implementations in J2eeProjectCapabilities class so the usage would look like compareProfileVersion(profile, J2eeProjectCapabilities.HIGHER_THAN_JAVA_EE_6)). Definitely most readable and extensible, but maybe a little bit over-designed for such simple use case.

Don't have strong opinion about what is the best one. What do you guys think?
Comment 9 Martin Janicek 2012-12-06 10:34:39 UTC
Created attachment 128946 [details]
Version one implementation
Comment 10 Martin Janicek 2012-12-06 10:35:24 UTC
Created attachment 128947 [details]
Version two implementation
Comment 11 Martin Fousek 2012-12-06 12:35:02 UTC
The usage of the way Nr.1 don't look pretty to me. Due to the attached patches I become fan of solution Nr.2 and Nr.3.

Nr.2: Looks really clear and simple, that's nice. Disadvantage I see in probable method count increase per JavaEE version/profile in this API. 

Nr.3: Right, can look overcomplicated, on the other side the usage remains clear to me (in comparsion to the Profile.JAVA_EE_6_FULL.isEqualOrHigher(Profile.JAVA_EE_6_WEB)) and has good extensibility. So once we will find out that comparsion WEB vs. FULL is necessary no additional method will be needed.

[MF04] Another thing... J2eeProjectCapabilities is project context specific - it means that it can take care about project's PROFILE, but I don't feel correct to take care about another PROFILEs using static method. So if we need/want static method for comparsion (project is no available) probably this method should be part of another friend API class.

[MF05] Bear on MF04, J2eeProjectCapabilities#isAtLeastJavaEE5() should be no-param method.

Anyway both solutions (2, 3) look good to me.
Comment 12 Petr Hejl 2012-12-06 13:15:40 UTC
Ok lets be a bit more pragmatic.

I suggest simple Util.isAtLeastJavaEE5() and Util.isAtLeastJavaEE6() for current usages. I think we all agree relying directly on Profile comparison should not be promoted and used in future.

With all different servers not strictly conform, optional parts of specification and recently EE platform pruning I think solutions sketched in javaee.specs.support should be followed.

What do you think?
Comment 13 Martin Janicek 2012-12-06 13:56:05 UTC
Created attachment 128960 [details]
Proposed solution

Based on your comments and offline discussion, I've created isAtLeastJavaEE5(Profile p) and isAtLeastJavaEE6(Profile p) methods in Util class (J2EE Support Utilities module).
Basically it's version two from comment 10 only with few implementation changes (now there is no need to change the method if someone add new Profile type).
Comment 14 Martin Janicek 2012-12-06 14:00:07 UTC
(In reply to comment #12)
> With all different servers not strictly conform, optional parts of
> specification and recently EE platform pruning I think solutions sketched in
> javaee.specs.support should be followed.
> 
> What do you think?

100% agree. The Util.isAtLeast... methods shouldn't be used when the code depends on the concrete Java EE spec. feature.
Comment 15 Martin Janicek 2012-12-06 14:41:21 UTC
Created attachment 128974 [details]
MF02 - @NonNull annotations added
Comment 16 Martin Fousek 2012-12-06 14:50:45 UTC
Patch looks well to me now.
Comment 17 David Konecny 2012-12-06 20:51:27 UTC
Thanks for doing this Martin. The API makes lots of sense to me.

DK01 - just for clarify and to avoid confusion it might be better to rename isAtLeastJavaEE6 to isAtLeastJavaEE6Web

DK02 - as EE7 profile is already defined in the trunk I would also suggest to add isAtLeastJavaEE7Web

DK03 - is isVersionEqualOrHigher public API or not? It is private in the patch but its Javadoc makes it look like it's meant to be public API.
Comment 18 David Konecny 2012-12-06 20:57:26 UTC
DK04 - if you accept DK01 then I wonder whether it would be useful to add also isAtLeastJavaEE6Full which would return true only for Java EE 6 Full and Java EE 7 Full profiles. Such method would be used for example in EJB module where only full profiles are acceptable.
Comment 19 Petr Hejl 2012-12-06 21:52:17 UTC
(In reply to comment #17)
> Thanks for doing this Martin. The API makes lots of sense to me.
> DK02 - as EE7 profile is already defined in the trunk I would also suggest to
> add isAtLeastJavaEE7Web
I there a real usage for that now? Looks like "just in case" method.
Comment 20 Petr Hejl 2012-12-06 21:57:14 UTC
(In reply to comment #18)
> DK04 - if you accept DK01 then I wonder whether it would be useful to add also
> isAtLeastJavaEE6Full which would return true only for Java EE 6 Full and Java
> EE 7 Full profiles. Such method would be used for example in EJB module where
> only full profiles are acceptable.
Davide please see my comment #12.

"I think we all agree relying directly on Profile comparison should not be promoted and used in future."

"...I think solutions sketched in javaee.specs.support should be followed..."

Looks like especially EJBSupport (already defined in javaee.specs.support) should be enhanced.
Comment 21 David Konecny 2012-12-06 22:13:27 UTC
Re. DK02 & "I there a real usage for that now?"

We will likely need it as part of EE7 support. I do not care if we add it now though or later - it is easy both ways.

Re. DK04 & "Looks like especially EJBSupport (already defined in javaee.specs.support) should be enhanced."

I thought that might be the answer. Fine with me. Thanks.
Comment 22 Martin Fousek 2012-12-06 23:46:06 UTC
(In reply to comment #20)
Re.Re. DK04:
> Looks like especially EJBSupport (already defined in javaee.specs.support)
> should be enhanced.

Agree that EJBSupport needs became more project oriented and also get one owful method rename. Actually whole javaee.specs.support looks too much J2eePlatform oriented right now and partially will be likely redesigned to be also project oriented with methods for checking both capabilities (project, server).

Re.Re. DK02: 100% agree with postponing.
Comment 23 Martin Janicek 2012-12-07 12:54:06 UTC
DK01: Ye, sure. I was thinking about the same just wasn't sure if it's better or not :)

DK02: I wouldn't do that if there isn't any real use case at the moment.

DK03: It's not supposed to be public. It was one of my previous implementation (letting up to the API client which version of Java EE he wants to compare with). But as the method use didn't look much readable we decided to use another implementation with two methods (one for Java EE 5 and one for Java EE 6 and higher). And thus I used it only as an internal implementation.

The important question that comes from DK02 and DK04 and that we already discussed in the office is if we really want to create such new methods for each use case we have. Because if we would, than the better solution would be something like version three from comment 8.
Petr's comment 12 summarize it well. We should use this API only for simplifying current code, but not as a general approach (javaee.specs.support should be rather used in most cases).
Comment 24 David Konecny 2012-12-09 21:05:13 UTC
(In reply to comment #23)
> We should use this API only for
> simplifying current code, but not as a general approach (javaee.specs.support
> should be rather used in most cases).

Makes sense.

I added a patch to issue 223442 which uses this new API to simplify some of code I introduced recently.
Comment 25 Martin Janicek 2012-12-11 07:50:51 UTC
If there are not more objections, I'm going to integrate later today.
Comment 26 Martin Janicek 2013-01-14 11:05:18 UTC
Fixed in (javaee7 branch): web-main #902b4e89749f
maven.j2ee usage: web-main #03c0362bdfb9

At first I thought I'll be able to change most of the usages to use the new API, but it shows up that in many cases it's hard to decide whether the semantic of isAtLeastJavaEE6Web should be used or whether only e.g. Java EE 6 should be a part of the condition.

I went through the j2ee.common friend modules and it seems that beside of the usages from David's 223442 issue, only these modules need to be updated:

ejbverification
beanvalidation
web.core
web.jsf
websvc.restapi

Based on the above I would like to kindly ask the owners of these modules to update usages of Profile class. Thanks in advance!
Comment 27 Quality Engineering 2013-02-13 01:51:40 UTC
Integrated into 'main-golden', will be available in build *201302122300* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/902b4e89749f
User: Martin Janicek <mjanicek@netbeans.org>
Log: #223317 - Profile should have method for better version comparison