Add a constructor to support tolerance for various time units #174
Conversation
ask-sdk-servlet-support/src/com/amazon/ask/servlet/verifiers/SkillRequestTimestampVerifier.java
Outdated
Show resolved
Hide resolved
...-servlet-support/tst/com/amazon/ask/servlet/verifiers/SkillRequestTimestampVerifierTest.java
Outdated
Show resolved
Hide resolved
...-servlet-support/tst/com/amazon/ask/servlet/verifiers/SkillRequestTimestampVerifierTest.java
Show resolved
Hide resolved
| * timeUnit must be non-null. | ||
| */ | ||
| public SkillRequestTimestampVerifier(long tolerance, TimeUnit timeUnit) { | ||
| this(ValidationUtils.assertNotNull(timeUnit, "timeUnit").toMillis(tolerance)); |
There was a problem hiding this comment.
Just one question. The SkillRequestTimestampVerifier constructor is called from the SkillServlet class: defaultVerifiers.add(new SkillRequestTimestampVerifier(timestampToleranceProperty != null ? timestampToleranceProperty : DEFAULT_TOLERANCE_MILLIS));
Therefore the old constructor will be called, or do you expect the developer to pass the verifiers in the SkillServlet constructor? Nevertheless the documentation should be adjusted here: https://developer.amazon.com/de/docs/custom-skills/host-a-custom-skill-as-a-web-service.html and here: https://github.com/alexa/skill-samples-java/blob/master/tidepooler/pom.xml
There was a problem hiding this comment.
The existing constructor behavior can't be changed for backwards compatibility reasons, so effectively to opt in to the new behavior one would need to pass an instance of the verifier to the servlet. Totally agree that the docs need to be updated as they still reference some v1 concepts.
There was a problem hiding this comment.
OK I thought of that, thanks for the explanation 👍
Issue: #173 Changes: Added overloaded constructor with an additional TimeUnit field which would convert the given tolerance to milli seconds Testing: Modified the test class to run with Parameterized.class to run all the existing test cases for the newly added constructor.
Description
Added overloaded constructor with an additional TimeUnit field which would convert the given tolerance to milli seconds
Motivation and Context
#173
Testing
Modified the test class to run with Parameterized.class to run all the existing test cases for the newly added constructor.
Types of changes
Checklist
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.