-
Notifications
You must be signed in to change notification settings - Fork 26
[PIRK 71] Move Querier creation logic into QuerierFactory #105
Conversation
-Added EncryptionPropertiesBuilder to make it easier to construct valid properties files for the QuerierFactory -Refactored the QuerierDriver to call QuerierFactory.
are tested as part of the rest of the standalone test suite.
+1 Works for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for this being a retrospective review.
Good improvements!
/** | ||
* Holds the various parameters related to creating a {@link Querier}. | ||
* | ||
* Created by ryan on 9/28/16. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop the author info.
DataSchemaLoader.initialize(); | ||
QuerySchemaLoader.initialize(); | ||
|
||
} catch (Exception e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be much better to handle the exception, or re-throw it, rather than consume and continue.
Why catch all Exception types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code was moved out of the old QuerierProps.validateQuerierProperties() method. I decided it would be safe to move out since this was the only place that method was ever called, and this code doing initialization, not validation. But I'm not familiar enough yet with what the schema loaders are doing to understand how to handle those exceptions differently. Any suggestions?
/** | ||
* Handles encrypting a query and constructing a {@link Querier} given a {@link EncryptionPropertiesBuilder}. | ||
* | ||
* Created by ryan on 9/28/16. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop author info in comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry! Will be more careful next time. Looks like @ellisonanne took care of this already as well.
public static boolean validateQuerierProperties() | ||
{ | ||
setGeneralDefaults(SystemConfiguration.getProperties()); | ||
if(validateGeneralQuerierProperties(SystemConfiguration.getProperties())) { | ||
String action = SystemConfiguration.getProperty(ACTION).toLowerCase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trivial: does not follow house formatting rules ;-)
I believe this has subsequently been fixed by @ellisonanne
.paillierBitSize(BaseTests.paillierBitSize) | ||
.certainty(BaseTests.certainty) | ||
.queryType(queryType) | ||
.build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
Added QuerierFactory and EncryptionPropertiesBuilder classes, which should make it easier to create a Querier object and return it directly to the caller, rather than having to write it to disk. This will make it easier for other projects to generate PIRK queries and pass them around.