search

PILLUTLAAVINASH / google-enterprise-connector-manager

Automatically exported from code.google.com/p/google-enterprise-connector-manager
0 stars 0 forks source link

AuthorizationResponse.equals should use the valid field #57

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
What steps will reproduce the problem?
1. Read the source. 

What is the expected output? What do you see instead?

The equals method should depend on both the docid and the valid state. As it 
is, two 
AuthorizationResponse instances will compare equal if one authorizes the 
document and the 
other does not.

It's OK for the hashCode method to ignore valid: if two objects are equal 
according to equals they 
must return the same hash codes, but the reverse is not required.

What version of the product are you using? On what operating system?

Connector Manager 1.0.2.

Please provide any additional information below.

Original issue reported on code.google.com by jl1615@gmail.com on 19 Oct 2007 at 1:24

GoogleCodeExporter commented 8 years ago 
I was just thinking that one reason to use the current implementation is that 
it appears to work better for set 
membership. That is, if I add AuthorizationResponse(true, "x") to a set, and 
then add AuthorizationResponse
(false, "x"), I might expect to have one entry in the set, and not two. I think 
this (admittedly straw-man) 
argument falls down in two ways. First, directly, I would also expect the false 
authorization to be in the set, 
but according to the rules of sets, the true authorization will remain in the 
set instead. Existing Set entries are 
preserved, unlike a Map, where the new entry overrides the old entry.

Second, indirectly, this implementation of equals is completely unintuitive 
when used by the contains or 
equals methods on a collection of any kind. I don't think that a collection 
containing AR(true, "x") should be 
considered equal to a collection containing AR(false, "x"), nor should the 
former be said to contain AR(false, 
"x").

I admit that it is interesting to ask whether a collection contains an 
AuthorizationResponse for a given docid, 
and it's interesting to build a set that can only contain a single 
AuthorizationResponse for a given docid, but I 
think you just have to work harder for those, and not use 
AuthorizationResponse.equals to provide them for 
you.

Original comment by jl1615@gmail.com on 19 Oct 2007 at 1:56

GoogleCodeExporter commented 8 years ago 
P1, because it's an incompatible change. This breaks code that assumes that the 
docid is all that matters for 
duplicate set and map entries, and for looking up entries in collections.

Original comment by jl1615@gmail.com on 19 Oct 2007 at 5:29

GoogleCodeExporter commented 8 years ago 
This is effectively an SPI change.  We should consider it in the next revision.

Original comment by donald.z...@gmail.com on 18 Apr 2008 at 10:22

GoogleCodeExporter commented 8 years ago

Original comment by jl1615@gmail.com on 8 Dec 2008 at 11:22

GoogleCodeExporter commented 8 years ago 
SPI change for the next release.

Original comment by jl1615@gmail.com on 9 Dec 2008 at 1:25

GoogleCodeExporter commented 8 years ago 
Fixed in Connector Manager revision r1425

Modified AuthorizationResponse.equals() so that two
AuthorizationResponse instances with the same docid,
but different isValid states are now considered inequal.

I also changed the AuthorizationResponse.hashCode() method
to fix a unit test that was flagged:
"TBD change the case below to assertFalse (and then make it work!)"

Change Log:
----------
M
projects/connector-manager/source/java/com/google/enterprise/connector/spi/Autho
rizationResponse.java
  - Changed the equals() method to compare the 'valid' fields.
  - Changed the hashCode() method to not be a straight hash of
    the docid, and include the 'valid' field in the calculation.

M
projects/connector-manager/source/java/com/google/enterprise/connector/spi/Autho
rizationManager.java
  - Modified SPI javadoc to indicate that the returned
    Collection of AuthorizationResponses need only contain
    positive responses.  This is not a change in behaviour,
    only a change in the documentation to reflect the actual
    behaviour.

M
projects/connector-manager/source/java/com/google/enterprise/connector/manager/P
roductionManager.java
  - Small optimization gathering positive AuthenticationResponses.

M
projects/connector-manager/source/javatests/com/google/enterprise/connector/spi/
AuthorizationResponseTest.java
  - Added License header.
  - Changed Assert. tests to TestCase asssertions.
  - Assertion that two AuthorizationResponses with the same
    docid but different valid flags are now inequal.
  - Assertion that two AuthorizationResponses with the same
    docid but different valid flags have different hashes.
  - Assertion that the hash of the AuthorizationResponse for a
    docid should be different than the hash of the docid itself.

Original comment by Brett.Mi...@gmail.com on 7 Jan 2009 at 6:58

GoogleCodeExporter commented 8 years ago

Original comment by jl1615@gmail.com on 12 Jan 2009 at 3:22