Add equals and hashCode to WindowsPrincipal #310

aleksandr-m · 2016-01-06T18:53:14Z

No description provided.

hazendaz · 2016-01-07T00:06:15Z

Can you squash commits and rename the test class back to the original maven compliant name....ie without 's'. We are on java 7 too so may be better to use objects class to do these. In the bigger scheme of things what is this solving? Can you add a change log as well?

Sent from Outlook Mobile

On Wed, Jan 6, 2016 at 10:53 AM -0800, "Aleksandr Mashchenko" notifications@github.com wrote:

You can view, comment on, or merge this pull request online at:

#310

-- Commit Summary --

Add equals and hashCode to WindowsPrincipal

-- File Changes --

M Source/JNA/waffle-jna/src/main/java/waffle/servlet/WindowsPrincipal.java (27)
D Source/JNA/waffle-jna/src/test/java/waffle/servlet/WindowsPrincipalTest.java (57)
A Source/JNA/waffle-jna/src/test/java/waffle/servlet/WindowsPrincipalTests.java (76)

-- Patch Links --

https://github.com/dblock/waffle/pull/310.patch
https://github.com/dblock/waffle/pull/310.diff

Reply to this email directly or view it on GitHub:
#310

aleksandr-m · 2016-01-07T17:45:50Z

Changelog added, commits squashed.

With current config maven includes tests with **/*Tests.java pattern.

In the bigger scheme of things what is this solving?
How do you compare principals w/o equals? Real life example: spring-security SessionRegistryImpl uses principal as a key in map for handling concurrent sessions.

hazendaz · 2016-01-07T23:33:42Z

Good catch! Legacy configuration for tests but required for now so thanks for flipping the test on in the build.

I'll get this merged later tonight. Thanks for the cotribution and further explanation.

Sent from Outlook Mobile

On Thu, Jan 7, 2016 at 9:45 AM -0800, "Aleksandr Mashchenko" notifications@github.com wrote:

Changelog added, commits squashed.

With current config maven includes tests with **/*Tests.java pattern.

In the bigger scheme of things what is this solving?
How do you compare principals w/o equals? Real life example: spring-security SessionRegistryImpl uses principal as a key in map for handling concurrent sessions.

Reply to this email directly or view it on GitHub:
#310 (comment)

hazendaz · 2016-01-10T05:05:10Z

I changed the master to include *_/_Test.java. Can you rename the test class back then I'll merge it. Ultimately I'm going to rename them all without 's' to comply with maven defaults and hence no reason for special configurations. Thanks.

hazendaz · 2016-01-10T05:07:59Z

For reference, no tests actually run CI builds. Travis CI because it's not windows and App Voyer as it has some other issues I haven't quite figured out.

sergey-podolsky · 2016-01-11T20:13:49Z

Source/JNA/waffle-jna/src/main/java/waffle/servlet/WindowsPrincipal.java

+     * @see java.lang.Object#equals(java.lang.Object)
+     */
+    @Override
+    public boolean equals(final Object o) {


Shouldn't the class be marked as final for immutability?

It appears it could be but that just stops extending it and overriding methods in it. Since it doesn't contain anything to change mutability other than a new instance it's probably ok. I'll have to look further to see why it has two non final fields. Feel free to investigate a bit further. Thanks.

Sent from Outlook Mobile

On Mon, Jan 11, 2016 at 12:13 PM -0800, "Sergey Podolsky" notifications@github.com wrote:

@@ -255,4 +255,30 @@ public String toString() {
return this.getName();
}

/*

\* (non-Javadoc)

\* @see java.lang.Object#equals(java.lang.Object)

*/

@OverRide

public boolean equals(final Object o) {

Shouldn't the class be marked as final for immutability?

Reply to this email directly or view it on GitHub:
https://github.com/dblock/waffle/pull/310/files#r49372072

Well, what I meant is that inheritance should be forbidden in general on a class that implements equals. One potential problem though is that it can break backwards compatibility for people who mock this class, but I can argue that data types should be stubbed rather than mocked.

aleksandr-m · 2016-01-12T19:55:17Z

IMO In the context of an app all principals should be the same type. So you don't have to compare oranges with apples. Take a look at spring-security classes which implement Principal they are not final and half of com.sun.security classes aren't final-s either.

Can you give a real life example when you need to compare principals and they might be of different types?

sergey-podolsky · 2016-01-12T20:48:35Z

The first random class I picked up from jdk that implements Prinicpal is
final:
http://www.docjar.com/html/api/javax/security/auth/kerberos/KerberosPrincipal.java.html
I am reluctant to check the rest of the classes implementing Principal,
especially outside of jdk, because I don't consider them as a golden source
of software engineering best practices. What I am saying is that it's
better to make public API safe in the first place instead of relying on
user's prudence.

On Tue, Jan 12, 2016 at 9:55 PM, Aleksandr Mashchenko <
notifications@github.com> wrote:

IMO In the context of an app all principals should be the same type. So
you don't have to compare oranges with apples. Take a look at
spring-security classes which implement Principal they are not final and
half of com.sun.security classes aren't final-s either.

Can you give a real life example when you need to compare principals and
they might be of different types?

—
Reply to this email directly or view it on GitHub
#310 (comment).

aleksandr-m · 2016-01-12T21:11:01Z

Usually I do want to extend principal to add some app special details to it. (Not sure if you can provide your own principal in waffle right now.) And don't forget that is fqn we are talking about, if it is the same then it shouldn't matter what type of principal exactly it is.

hazendaz · 2016-01-13T03:21:39Z

@dblock Do you want to chime in on this conversation? As far as the original PR, It's ready to merge but wanted you to have a chance to chime in.

@aleksandr-m I don't see any harm in this going in as-is at this point. I'm not too overly concerned on mutability even if @equals is overridden. Even in that case, the new class is going to expect it is overridden as well, either including the super or not. And obviously you want to ultimately extend it anyways so blocking you wouldn't be that nice. I would probably still use new Objects class to setup hashcode/equals since it's a little more conside even though this is small and further takes us out of really setting that up.

@sergey-podolsky lombokproject handles this type of situation quite well and we certainly could add that here. Very smart guy behind that project. I agree with all the points made but want to get @dblock to chime in.

I did review the class as it stands currently. There was just one field for identity that was not listed as final but it should be. As far as mock testing, jmockit can mock finals so no issues there even if this were to be finalized.

Now interesting enough, the intereface calls out equals / hashcode / toString. Now while it doesn't then enforce overriddiing it becuase of what those exactly are, I sort of expect the original Principal auther in the java.security package was expecting users to override it explicitely. This further makes me think this is needed but I don't know that we need to go to the level fo blocking extending this class.

dblock · 2016-01-13T12:08:06Z

CHANGELOG.md

@@ -11,6 +11,7 @@
 * [#268](https://github.com/dblock/waffle/pull/301): Cannot log in automatically on machine where Tomcat service is running
 * [#274](https://github.com/dblock/waffle/pull/274): Update WindowsSecurityContextImpl.java to handle SEC_E_BUFFER_TOO_SMALL
 * [#128](https://github.com/dblock/waffle/pull/128): Update WindowsSecurityContext.cs to handle SEC_E_BUFFER_TOO_SMALL
+* [#310](https://github.com/dblock/waffle/pull/310): Add equals and hashCode to WindowsPrincipal


Sup with this changelog losing all the convention of having a period at the end and the name of the contributor? Maybe not in this PR, but someone please make it look more consistent :)

I can look into fixing it. I'd love to automate it. I'll look into that as well. It's otherwise prone to human error ;)

Sent from Outlook Mobile

On Wed, Jan 13, 2016 at 4:08 AM -0800, "Daniel Doubrovkine (dB.) @dblockdotorg" notifications@github.com wrote:

@@ -11,6 +11,7 @@

#268: Cannot log in automatically on machine where Tomcat service is running

#274: Update WindowsSecurityContextImpl.java to handle SEC_E_BUFFER_TOO_SMALL

#128: Update WindowsSecurityContext.cs to handle SEC_E_BUFFER_TOO_SMALL
+* #310: Add equals and hashCode to WindowsPrincipal

Sup with this changelog losing all the convention of having a period at the end and the name of the contributor? Maybe not in this PR, but someone please make it look more consistent :)

Reply to this email directly or view it on GitHub:
https://github.com/dblock/waffle/pull/310/files#r49582512

👍

Personally I think automation makes it hard to read. I generally pay a lot of attention to change logs in projects and I want something closer to a human explanation of what has changed. Just my 0.02c.

dblock · 2016-01-13T12:12:07Z

So the discussion is about making WindowsPrincipal final? I think I saw people inheriting WindowsPrincipal, but I don't have a strong opinion - I suggest that change gets opened as a separate PR with proper UPGRADING and what not, so people can chime in.

hazendaz · 2016-01-17T18:40:50Z

I agree separate PR needed if we were to make class final. That can harm backwards compatibility. I'm merging this otherwise.

…uals_hash_code Add equals and hashCode to WindowsPrincipal

aleksandr-m force-pushed the feature/windows_principal_equals_hash_code branch from 265b1e5 to 7eee2f0 Compare January 7, 2016 17:33

Add equals and hashCode to WindowsPrincipal

88b149f

aleksandr-m force-pushed the feature/windows_principal_equals_hash_code branch from 7eee2f0 to 88b149f Compare January 10, 2016 14:49

sergey-podolsky reviewed Jan 11, 2016
View reviewed changes

dblock reviewed Jan 13, 2016
View reviewed changes

hazendaz added a commit that referenced this pull request Jan 17, 2016

Merge pull request #310 from aleksandr-m/feature/windows_principal_eq…

c2fbdf9

…uals_hash_code Add equals and hashCode to WindowsPrincipal

hazendaz merged commit c2fbdf9 into Waffle:master Jan 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add equals and hashCode to WindowsPrincipal #310

Add equals and hashCode to WindowsPrincipal #310

aleksandr-m commented Jan 6, 2016

hazendaz commented Jan 7, 2016

aleksandr-m commented Jan 7, 2016

hazendaz commented Jan 7, 2016

hazendaz commented Jan 10, 2016

hazendaz commented Jan 10, 2016

sergey-podolsky Jan 11, 2016

hazendaz Jan 11, 2016

sergey-podolsky Jan 12, 2016

aleksandr-m commented Jan 12, 2016

sergey-podolsky commented Jan 12, 2016

aleksandr-m commented Jan 12, 2016

hazendaz commented Jan 13, 2016

dblock Jan 13, 2016

hazendaz Jan 13, 2016

dblock Jan 13, 2016

dblock commented Jan 13, 2016

hazendaz commented Jan 17, 2016

Add equals and hashCode to WindowsPrincipal #310

Add equals and hashCode to WindowsPrincipal #310

Conversation

aleksandr-m commented Jan 6, 2016

hazendaz commented Jan 7, 2016

aleksandr-m commented Jan 7, 2016

hazendaz commented Jan 7, 2016

hazendaz commented Jan 10, 2016

hazendaz commented Jan 10, 2016

sergey-podolsky Jan 11, 2016

Choose a reason for hiding this comment

hazendaz Jan 11, 2016

Choose a reason for hiding this comment

sergey-podolsky Jan 12, 2016

Choose a reason for hiding this comment

aleksandr-m commented Jan 12, 2016

sergey-podolsky commented Jan 12, 2016

aleksandr-m commented Jan 12, 2016

hazendaz commented Jan 13, 2016

dblock Jan 13, 2016

Choose a reason for hiding this comment

hazendaz Jan 13, 2016

Choose a reason for hiding this comment

dblock Jan 13, 2016

Choose a reason for hiding this comment

dblock commented Jan 13, 2016

hazendaz commented Jan 17, 2016