Forbid mismatched inner element in XML flow inputs#3104
Conversation
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on gbrodman).
core/src/main/java/google/registry/flows/picker/FlowPicker.java line 259 at r1 (raw file):
throw new MissingCommandException(); } if (innerCommand instanceof ResourceCommandWrapper resourceCommandWrapper) {
I guess I don't understand what this is protecting against. Before this check was added, what was it doing? Surely the test XML of a create inside a check would fail?
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on CydeWeys).
core/src/main/java/google/registry/flows/picker/FlowPicker.java line 259 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
I guess I don't understand what this is protecting against. Before this check was added, what was it doing? Surely the test XML of a create inside a check would fail?
apparently that wouldn't fail! it's not catastrophic, but that was theoretically possible before and it'd make the metrics slightly incorrect e
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on gbrodman).
core/src/main/java/google/registry/flows/picker/FlowPicker.java line 259 at r1 (raw file):
Previously, gbrodman wrote…
apparently that wouldn't fail! it's not catastrophic, but that was theoretically possible before and it'd make the metrics slightly incorrect e
OK but if it wasn't failing, then what was actually happening? What does a create inside of a check do?
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on CydeWeys).
core/src/main/java/google/registry/flows/picker/FlowPicker.java line 259 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
OK but if it wasn't failing, then what was actually happening? What does a create inside of a check do?
It would perform the create, but our metrics would mark it as a check because the metrics use the outer command type instead of any possible inner commands that might exist
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
core/src/main/java/google/registry/flows/picker/FlowPicker.java line 259 at r1 (raw file):
Previously, gbrodman wrote…
It would perform the create, but our metrics would mark it as a check because the metrics use the outer command type instead of any possible inner commands that might exist
Hrm, so we were basically just ignoring the inner command type. Huh.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on gbrodman).
This change is