Add equals and hashCode to WindowsPrincipal

[aleksandr-m]

No description provided.

[hazendaz]

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]

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]

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]

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]

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]

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

[hazendaz]

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

[sergey-podolsky]

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]

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]

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]

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]

@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]

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 :)

[hazendaz]

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

[dblock]

👍

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]

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]

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