ESD-1703: Fix MCR prefix filter list ge/le encoding - #177
Open
Phil-Browne wants to merge 6 commits into
Open
Conversation
The outbound path marshaled the user-facing MCRPrefixFilterList straight onto the wire, so ge/le went out as JSON numbers where PrefixEntryDto declares strings, and omitempty on an int meant a deliberate 0 produced no key at all. NAT gateway prefix lists dropped the zero too, via an explicit `if e.Ge > 0` in toAPI. Add a toAPI converter for MCRPrefixFilterList mirroring the existing inbound one, and call it from create and modify. Ge/Le on MCRPrefixListEntry and NATGatewayPrefixListEntry become *int so an unset field is distinguishable from a deliberate 0, which megalith forwards to netauto as a distinct value rather than defaulting. Tests assert the marshaled request body rather than a stubbed response.
The new MCR converter allocated its entries slice unconditionally, so a list with a nil Entries marshaled as "entries": [] where direct marshaling produced null. On the PUT that turns a payload the API should reject into a well-formed "replace with zero entries" request, so restore null. Guard the inbound converter against a null entry in the response. It dereferenced a nil element and panicked, and the outbound path now emits exactly that shape, so the two directions have to agree. Cover the gaps the review found by mutation: le set to 0 on both encode and decode for MCR and NAT (only ge was covered, so the same bug on le would have shipped), the NAT gateway PUT body (which had no wire-level coverage at all), and the nil list, nil Entries and nil entry shapes. Tell consumers in the CHANGELOG to map unset to nil rather than PtrTo(0). A 0 used to be dropped and is now honored, so a mechanical migration would silently change which prefixes a filter matches.
Round two aimed at the fix commit itself. Six changes: - Reject a null prefix list entry on decode instead of returning a nil element. The earlier guard only moved the panic into every caller. - Wrap decode errors with the entry index and the offending field. - Share prefixLenToAPI / prefixLenFromAPI between MCR and NAT rather than repeating the same rule in four places. - Cover the decode error branches: non-numeric ge, non-numeric le, and a null entry on MCR, plus a non-numeric le on NAT. - Rewrite the CHANGELOG entry, which was longer than the one review called too long and claimed a negatives fix MCR never had. - Reword the integration-test ge comment as an observation, not a contract the spec does not declare.
…products Round three of review found the read and write paths disagreeing with each other. The read path rejects a null entry as malformed while toAPI still emitted one, so the SDK could write a list its own read path then refused forever. megalith validates @NotNull on the entries list only, so a null element passes validation and faults further in; there is nothing to preserve. toAPI now returns an error instead. NAT typed its wire entries by value, so a null decoded to a blank entry and survived a read-modify-write as {"action":"","prefix":""}. Both products share one schema, so both now reject it. Also: a null entries array no longer decodes to a non-nil empty slice, which made the outbound nil-preservation fix unreachable through a GET. Drop the CHANGELOG's claim that le: 0 matches nothing, which the spec's own PrefixEntryDto description contradicts (0 is documented in range, its semantics unstated). Pin the entry index, the list id passthrough and NAT's ge wording, all of which survived mutation before.
… null
Verified against staging: both bodies toAPI could emit are 400s. A bare
null and {"entries":null} are rejected, and the tests were pinning them as
expected output. Fail locally instead and spend no round trip on them.
An empty entries array is left alone: the API accepts it and clears the
list, which is a legitimate request.
Also corrects the changelog, which claimed a stray 0 changes which routes
a filter matches. Staging enforces prefixLen <= ge <= le, so a 0 below the
prefix length is a 400.
Mutation testing found several assertions that did not bite. Entry-index errors were pinned at index 0, where an off-by-one in the loop counter survives, and the create and update read-backs had no null-entry coverage of their own. Decode assertions now cover the fields that ride along with ge/le rather than ge/le alone. Fixture prefixes now match what staging accepts: a ge or le of 0 is only legal on a default route. Comments lose the justification they duplicated from the decision site.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SDK marshals
ge/lewith the wrong type on MCR prefix filter list create and modify.MCRPrefixFilterListgoes straight on the wire, and its entries holdGe int/Le intwithomitempty, so values go out as JSON numbers where the API declares strings, and a prefix length of 0 produces no key at all. NAT gateway prefix lists get the string conversion right but drop the zero with an explicitif e.Ge > 0.megalith does not default an absent
ge: it forwards the field verbatim into netauto underNON_NULL. Absent and"0"are genuinely different requests downstream, so dropping a deliberate 0 is a behavior bug, not only a contract mismatch.MCRPrefixFilterList.toAPI(), mirroring the existing inbound converter, and call it fromCreatePrefixFilterListandModifyMCRPrefixFilterList.MCRPrefixListEntry.Ge/.LeandNATGatewayPrefixListEntry.Ge/.Lebecome*intso unset is distinguishable from a deliberate 0. Breaking for both consumers.toAPIemits"0"for a set zero instead of dropping it.prefixLenToAPI/prefixLenFromAPI) rather than repeating the same rule in four places.ge/lethat is not a number and on anullentry, naming the index. Previously a bad value zeroed the field and anullentry panicked.toAPIrefuses to send anullentry, so the SDK cannot write a list its own read path then rejects. megalith validates the entries list but not its elements, so anullfaults it rather than earning a 400.Migrating a consumer
Wrapping the existing
intfields inPtrTo(...)is not a safe migration. Both consumers encode "unset" as0, and on the provider side that is documented behavior, not an accident.terraform-provider-megaport's NAT prefix list schema declaresge/leasOptional + ComputedwithDefault: int64default.StaticInt64(0), described to users as "Omit or set to 0 to match the prefix's own length". The SDK'sif e.Ge > 0was that sentinel's only implementation. With it gone, every entry the user left unbounded sends a realle: "0"instead of nothing, which is a different request.nil" is not reachable from that schema as written: an attribute with a static default never reportsIsNull(), and returning a null on read where the plan holds 0 fails the apply with "Provider produced inconsistent result after apply". A correct migration has to drop the defaults as well, which is a second breaking change for provider users.megaport-cli(mcr_inputs.go) flattens its own pointer to a0default, and the provider'smcr_prefix_filter_list_utils.gotestsif entry.Ge == 0 || entry.Le == 0.Those updates land in their own tickets. Treat the provider one as a gate on bumping the SDK there, not a follow-up.
What I could not verify
Ge: 0toGe: nil. The API looks like it omitsgefrom the response when it equals the prefix length: the pre-existing test sendsGe: 24on a/24and expected0back. Under the oldinttype,0meant "absent or zero", so the expectation was ambiguous. Two risks here, both needing a credentialed run: now that a correctly typed"24"goes out, the API may start echoinggeback and these assertions would fail; and if the API ever returns"ge": "0",nilis the wrong expectation.ge/leanywhere in the declared contract: the spec types them as plainstringwith no pattern or bounds (its description names 0 to 32 and 0 to 128, but nothing checks it), and megalith holds them as opaque nullable strings. So a negative value is more likely forwarded to netauto than rejected, andge > lehas never been checked. Range validation is deliberately out of scope here.ge/leof 0 actually does. The spec's description puts 0 inside the valid range and says nothing about its meaning, megalith forwards the string untouched, and netauto declares a bareint32. This is the highest-consequence unknown and the reason the provider migration is a gate: a consumer that migrates a stray0sends adenynobody has characterized.PrefixListRequestdeclares noidproperty, but the SDK sends"id": 0on create and the body's own id on modify (which can disagree with the path). Pre-existing and left alone per the ticket's scope; the spec sets noadditionalProperties: falseand megalith ignores unknown properties, so it is harmless today. Worth its own ticket.