New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule result metadata #71

Merged
merged 5 commits into from Dec 19, 2017

Conversation

Projects
None yet
2 participants
@dfish3r
Copy link
Member

dfish3r commented Dec 18, 2017

No description provided.

dfish3r added some commits Dec 18, 2017

Add RuleResultMetadata component.
Metadata is used to communicate properties about the password which are related to the rule back to the validator.
This allows the validator to give feedback or advice to users attempting to create passwords which meet the rule requirements.
Note that metadata is returned for both valid and invalid password results.
Update WhitespaceRule to allow custom whitespace characters.
@serac
Copy link
Member

serac left a comment

The only real substantial concern I had were the metadata categories. I'd like to see some facility for making those known at compile time.

@@ -111,6 +111,7 @@ public RuleResult validate(final PasswordData passwordData)
result.setValid(false);
result.getDetails().addAll(rr.getDetails());
}
result.getMetadata().putAll(rr.getMetadata().getAll());

This comment has been minimized.

@serac

serac Dec 19, 2017

Member

Do you want to accommodate value merging here? I don't believe we have a case at present where two different rules produce the same key, but in that case the implementation above would cause the latter to overwrite the former. Merge semantics would be preferable. On the other hand we can say "don't do that."

This comment has been minimized.

@dfish3r

dfish3r Dec 19, 2017

Member

I prefer don't do that. Rules participating in metadata should produce unique keys.

This comment has been minimized.

@serac

serac Dec 19, 2017

Member

That's fine.

*/
public Map<String, Object> getAll()
{
return Collections.unmodifiableMap(metadata);

This comment has been minimized.

@serac

serac Dec 19, 2017

Member

Why not simply expose the map for mutations? Since you can mutate the underlying map with the put operations, returning an unmodifiable collection here doesn't provide much if anything. The type-sensitive getter has some value I suppose, but you only have a use case for Map<String, Integer> today, so you could just use that signature.

This comment has been minimized.

@dfish3r

dfish3r Dec 19, 2017

Member

Since this class only wraps a Map I made as well provide some API value by controlling the mutators. That's the only reason for making it unmodifiable.

If we want to say all metadata is an Integer, I can make that change.

This comment has been minimized.

@serac

serac Dec 19, 2017

Member

+1 to change to Integer. Then the getter can return Integer (or null) without any fancy type checking.

{

/** Additional metadata that provide information about the rule. */
protected final Map<String, Object> metadata;

This comment has been minimized.

@serac

serac Dec 19, 2017

Member

I can appreciate the flexibility of a map of objects, but it deprives clients of knowing about what should be well-defined metadata categories at compile time. I'm in favor of getters for the categories, but maybe we can split the difference with an enum for the categories. Then you could have put methods for both well-known categories by enum as well as ad hoc categories by string. In that view the underlying map could still be keyed on a string.

This comment has been minimized.

@dfish3r

dfish3r Dec 19, 2017

Member

Concrete metadata categories places a high burden on some of the rules to implement logic to determine which category metadata falls into. See CharacterRule. I didn't want metadata code bleeding into the CharacterData classes, but that would be likely result of this sort of change.

This comment has been minimized.

@dfish3r

dfish3r Dec 19, 2017

Member

Actually, if we're willing to drive this via naming convention...I could make the category names line up with the EnglishCharacterData enums in such a way to make this work.
I'll investigate.

This comment has been minimized.

@serac

serac Dec 19, 2017

Member

That approach sounds promising.

This comment has been minimized.

@dfish3r

dfish3r Dec 19, 2017

Member

Pushed a change for this.

{
final Map<String, Object> m = new LinkedHashMap<>();
m.put(
"whitespaceCharacterCount",

This comment has been minimized.

@serac

serac Dec 19, 2017

Member

I think these keys are overly wordy. whitespaceCount seems perfectly clear and more succinct besides.

This comment has been minimized.

@dfish3r

dfish3r Dec 19, 2017

Member

I can change that.

{
return type.cast(metadata.get(key));
if (value < 0) {
throw new IllegalArgumentException("Count value must be greater than zero");

This comment has been minimized.

@serac

serac Dec 19, 2017

Member

Zero count is allowed, so error message is misleading. "Count must be non-negative" seems sufficiently clear and correct. Also, might add a note about allowed range to the value parameter: @param value non-negative character count.

This comment has been minimized.

@dfish3r

dfish3r Dec 19, 2017

Member

Fixed.

@serac

This comment has been minimized.

Copy link
Member

serac commented Dec 19, 2017

Use of enums looks good.

@serac serac merged commit 1486661 into master Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment