A features value that is not a list is refused - #86
Merged
Merged
Conversation
yield from over a mapping yields its keys and len() of it counts them, so a
page whose features is an object made the walk emit the field names as though
they were features and step its offset by the number of them. Nothing raised.
Measured on the unmodified tree before the fix: a page carrying
{"features": {"OBJECTID": 1, "YEAR_": 2020}} makes iter_features yield
['OBJECTID', 'YEAR_'] and stop, after one request.
Found by the consuming project's own copy of this walk, which checks the type
and would have had to keep the check to move onto this one. A compensation
cannot be retired until the thing it compensates for is gone, so the check
belongs here rather than downstream.
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.
What was wrong
iter_featuresreadspayload["features"], yields from it, and steps its offset by its length. Both of those do something plausible to a mapping:yield fromover a dict yields its keys, andlen()of it counts them. So a page whosefeaturesis an object made the walk emit the field names as though they were features and step its offset by the number of them, and nothing raised.Measured on unmodified
origin/main, with_getsubstituted and a page carrying{"features": {"OBJECTID": 1, "YEAR_": 2020}}:That is the whole finding, and it is stronger than any sabotage because nothing about it was arranged.
How it was found
wildfire-service-territory-overlapkeeps its own paged walk,fetch_feature_pages, which itsdocs/UPSTREAM.mdrecords as a compensation for this package not exposing an output format. #84 closed that, and moving that project ontoiter_featureswould mean deleting the local walk. Its local walk does one thing this one did not:A compensation cannot be retired until the thing it compensates for is gone, so keeping that check downstream would leave a fragment of the duplication in place for no reason. The check belongs here.
What changed
iter_featuresrefuses afeaturesvalue that is not a list, naming the type it got and the offset it was at, and saying why a mapping in particular is dangerous rather than merely wrong. Nine lines and a test.payload["features"]being absent is already refused, from #84. This is the other half of the same question: a key that is present and is not a page.How it was verified
make verifygreen in full, determinism gate included. The measurement above is the evidence; a deliberate sabotage here would only be a restatement of it.Part of the upstream contribution loop
wildfire-service-territory-overlap's roadmap item 4.4 describes. That project pins a commit, so this reaches it only when the pin moves, deliberately.Prepared with AI assistance; reviewed before submission.