Normalize Fee extension XML tags in EPP response #2953
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
core/src/main/java/google/registry/flows/FeeExtensionXmlTagNormalizer.java
Fixed
Show fixed
Hide fixed
core/src/test/java/google/registry/flows/FeeExtensionXmlTagNormalizerTest.java
Fixed
Show fixed
Hide fixed
weiminyu
force-pushed
the
epp-namespace-norm
branch
2 times, most recently
from
168443b to
d1f9e95
Compare
weiminyu
left a comment
weiminyu
left a comment
There was a problem hiding this comment.
@weiminyu resolved 2 discussions.
Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on @CydeWeys and @gbrodman).
CydeWeys
left a comment
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 2 comments.
Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @gbrodman and @weiminyu).
core/src/main/java/google/registry/flows/FeeExtensionXmlTagNormalizer.java line 55 at r2 (raw file):
* * <p>Some registrars are not XML namespace-aware and rely on the XML tags being specific literals. * This makes it impossible to perform seamless rollout of new versions. Nomulus and the all
Grammar error here
core/src/main/java/google/registry/flows/FeeExtensionXmlTagNormalizer.java line 63 at r2 (raw file):
* versions from the message, thus freeing up the canonical tag ('fee') for the active version. */ public class FeeExtensionXmlTagNormalizer {
What are the performance implications of this approach? This is running a nontrivial amount of XML parsing code for each command response we return, right?
weiminyu
left a comment
weiminyu
left a comment
There was a problem hiding this comment.
@weiminyu made 2 comments.
Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @gbrodman).
core/src/main/java/google/registry/flows/FeeExtensionXmlTagNormalizer.java line 55 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Grammar error here
Done.
core/src/main/java/google/registry/flows/FeeExtensionXmlTagNormalizer.java line 63 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
What are the performance implications of this approach? This is running a nontrivial amount of XML parsing code for each command response we return, right?
This is on par with the XmlSanitier that masks passwords: actions range between a few milliseconds to 10s of milliseconds.
Since the input is produced by our EppXmlTransformer, we know the format and can use regex. Rewrote it and processing is pretty consistent at 1 - 2 milliseconds per call, and it scales well with larger messages.
| @ParameterizedTest(name = "normalize_withFeeExtension-{0}") | ||
| @MethodSource("provideTestCombinations") | ||
| @SuppressWarnings("unused") // Parameter 'name' is part of test case name | ||
| void normalize_withFeeExtension(String name, String inputXmlFilename, String expectedXmlFilename) |
Check notice
Code scanning / CodeQL
Useless parameter Note test
There was a problem hiding this comment.
Useless warning. Not seeing the suppresswarning annotation
weiminyu
left a comment
weiminyu
left a comment
There was a problem hiding this comment.
@weiminyu made 1 comment.
Reviewable status: 0 of 13 files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @gbrodman).
| @ParameterizedTest(name = "normalize_withFeeExtension-{0}") | ||
| @MethodSource("provideTestCombinations") | ||
| @SuppressWarnings("unused") // Parameter 'name' is part of test case name | ||
| void normalize_withFeeExtension(String name, String inputXmlFilename, String expectedXmlFilename) |
There was a problem hiding this comment.
Useless warning. Not seeing the suppresswarning annotation
weiminyu
left a comment
weiminyu
left a comment
There was a problem hiding this comment.
@weiminyu resolved 1 discussion.
Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @gbrodman).
Nomulus currently supports multiple versions of the Fee extensions. Our current tooling requires that each version must use a unique namespace tag, e.g., fee11, fee12, etc. Some client registrars are sensitive to the tag literal used by the version of the extension they use. For example, a few registrars currently using v0.6 have requested that the `fee` literal be used on the versions they currently use. With registrars upgrading at their own schedule, this kind of requests are impossible to satisfy. This PR instroduces a namespace normalizer class for EPP responses. The key optimization is that each EPP response never mixes multiple versions of a service extension. Therefore we can define a canonical tag for each extension, and change the tag of the extension in use in a response to that. This normalizer only handles Fee extensions right now, but the idea can be extended to others if use cases come up. This normalizer will be applied to all flows in a future PR.
b/478848482
weiminyu
force-pushed
the
epp-namespace-norm
branch
from
00e3553 to
7ad961a
Compare
2 participants
Nomulus currently supports multiple versions of the Fee extensions. Our current tooling requires that each version must use a unique namespace tag, e.g., fee11, fee12, etc.
Some client registrars are sensitive to the tag literal used by the version of the extension they use. For example, a few registrars currently using v0.6 have requested that the
feeliteral be used on the versions they currently use. With registrars upgrading at their own schedule, this kind of requests are impossible to satisfy.This PR instroduces a namespace normalizer class for EPP responses. The key optimization is that each EPP response never mixes multiple versions of a service extension. Therefore we can define a canonical tag for each extension, and change the tag of the extension in use in a response to that. This normalizer only handles Fee extensions right now, but the idea can be extended to others if use cases come up.
This normalizer will be applied to all flows in a future PR.
See b/478848482
This change is