PIRK-79 #115
PIRK-79 #115
Conversation
…n on querier and response.
| public void testQuerierResponseSerializationDeserialization() | ||
| { | ||
| testQuerierResponseSerializationDeserialization(jsonSerializer); | ||
| testQuerierResponseSerializationDeserialization(javaSerializer); |
There was a problem hiding this comment.
Is this line supposed to be here twice? If so, a comment as to why would be nice.
There was a problem hiding this comment.
That line isn't in there twice. It's in there once with the arg jsonSerializer, and once with the arg javaSerializer. ;) Thus are both extant serialization services tested.
There was a problem hiding this comment.
My tired brain took too many shortcuts…
| if (!NSquared.equals(paillier.NSquared)) | ||
| return false; | ||
| if (!lambdaN.equals(paillier.lambdaN)) | ||
| return false; |
There was a problem hiding this comment.
Can all of the above if stmts be OR'ed into one single stmt that returns false ?
There was a problem hiding this comment.
I can, but it just leaves a blob of ORd-together stuff. I admit to knowing little about the java compiler; wouldn't it just optimize these into such a beast anyway? If so, then I feel like it's more aesthetically pleasing to just leave them separated. If not, well, screw that, and I'll change it; or if the community feels my subjective aesthetics aren't aesthetically pleasing.
There was a problem hiding this comment.
Personally I prefer the separate if statements when there are this many entries because it's easier to skim through. Putting all of those statements on one line makes it easy to visually glide past one of them.
| result = 31 * result + w.hashCode(); | ||
| result = 31 * result + bitLength; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Java 8 provides a Objects.hash(). Could we use that for all hashCode() methods ?
There was a problem hiding this comment.
Certainly one could. My IDE auto-generated these, and after that I just checked them over to make sure they made sense.
Honestly, Objects.hash really is kind of pretty. I'll change this over to use it for all the hashCode methods I generated for this code.
While I'm doing that, and since a quick google didn't seem to produce useful results (or it could be me; the hour is late), is Objects.hash going to Do The Right Thing when I feed it primitives?
| if (!queryInfo.equals(response.queryInfo)) | ||
| return false; | ||
| return responseElements.equals(response.responseElements); | ||
|
|
There was a problem hiding this comment.
Can we use Java 8 - Objects.equal() functionality for all equals() methods ?
There was a problem hiding this comment.
We could indeed replace every instance of a.equals(b) with Objects.equals(a,b). I feel this brooks us very little. In exchange for regex-search-replacing all the a.equals(b) with Objects.equals(a,b), we gain...the ability to omit some auto-generated code from the beginning of the equals method. If the community really feels we should do this (or, heck, smarthi, if you feel strongly about it; my opinion on this matter lacks fire, and is easily swayed), I will of course be willing, but it doesn't simplify the code much, and that simplification takes far more time than it took for me to "Cmd-N -> equals() and hashCode()". This difference is not nearly so great as that for Objects.hash, which just really makes things a lot cleaner and makes it harder to accidentally type in the wrong place and subtly break ones hashCode.
| @@ -51,9 +51,9 @@ | |||
| public static final int dataPartitionBitSize = 8; | |||
|
|
|||
| // Selectors for domain and IP queries, queryIdentifier is the first entry for file generation | |||
| private static ArrayList<String> selectorsDomain = new ArrayList<>( | |||
| public static ArrayList<String> selectorsDomain = new ArrayList<>( | |||
There was a problem hiding this comment.
generic interface List<> on LHS.
There was a problem hiding this comment.
Indeed! Good catch. Will change.
| Arrays.asList("s.t.u.net", "d.e.com", "r.r.r.r", "a.b.c.com", "something.else", "x.y.net")); | ||
| private static ArrayList<String> selectorsIP = new ArrayList<>(Arrays.asList("55.55.55.55", "5.6.7.8", "10.20.30.40", "13.14.15.16", "21.22.23.24")); | ||
| public static ArrayList<String> selectorsIP = new ArrayList<>(Arrays.asList("55.55.55.55", "5.6.7.8", "10.20.30.40", "13.14.15.16", "21.22.23.24")); |
There was a problem hiding this comment.
Generic interface List<> on LHS
|
+1 |
This PR is to address the notional problem that the QueryDeserializer I wrote would throw an exception if it received a request to deserialize a query which didn't have a full schema included, only a name. I realized that Pirk should be expected to handle such a circumstance, and that my code wouldn't.
Ironically, the best way of fixing this was to eliminate my custom deserializer. I realized this when I couldn't get changes to the QueryDeserializer code to affect anything. I looked closely at the JsonSerializer and realized my IDE had auto-imported the wrong "Query" (thanks, Intellij!) and so the custom class wasn't actually performing the deserialization. This...I'll be honest, it felt embarassing, and so I share it with you, to simultaneously embiggen and diffuse the embarassment.
Along the way, I did some small bug fixing and, more importantly, fleshed out SerializationTest. We now actually test serializing and deserializing Response and Querier objects (and, along the way, Query, QueryInfo, QuerySchema, and Paillier objects), for both serializers, java and json.