Reject PUT changes to ImportList.MinRefreshInterval instead of discarding them - #86
Open
jbob06 wants to merge 1 commit into
Open
Conversation
…ding them MinRefreshInterval is a fixed, per-list-type constant declared on each IImportList implementation (e.g. GoodreadsBookshelf => 12 hours) and is intentionally excluded from persistence (TableMapping ignores it). ImportListFactory.SetProviderCharacteristics unconditionally overwrites it from the provider on every load, so a PUT that changed it was silently accepted (202) with the change discarded - the response goes on to echo the provider's value, not the one that was requested. Add a PutValidator rule that compares the requested value against the provider's authoritative value and returns a clear validation error when they differ, instead of a silent no-op. TimeSpan.Zero (what a non-nullable TimeSpan field deserializes to when omitted from the request body) is always allowed through, since no provider ever uses zero and clients that only send the fields they mean to change shouldn't get a spurious 400. An unknown id is left to the normal Get()-based 404 handling rather than surfacing as a validation error. Fixes Chaptarr#23
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.
Summary
Fixes #23.
ImportListDefinition.MinRefreshIntervalis a fixed, per-list-type constant declared on eachIImportListimplementation (e.g.GoodreadsBookshelf => TimeSpan.FromHours(12)), intentionally excluded from persistence (TableMapping.cshas.Ignore(i => i.MinRefreshInterval)), and unconditionally overwritten from the provider on every load viaImportListFactory.SetProviderCharacteristics. So aPUT /api/v1/importlist/{id}that changed it was silently accepted (HTTP 202), with the change discarded - the response goes on to echo the provider's value, not the one that was requested.This adds a
PutValidatorrule comparing the requested value against the provider's authoritative value (via_importListFactory.Find(id)+SetProviderCharacteristics) and rejects the PUT with a clear validation error when they differ, instead of silently discarding it.Two things to flag:
TimeSpan.Zerois always allowed through, sinceMinRefreshIntervalis a non-nullableTimeSpanand a client that omits the field from its request body gets zero on deserialization. No provider ever uses zero, so this can't mask a real attempt to change the value, and it avoids a spurious 400 for clients that only send the fields they mean to change.Get()-based 404 handling rather than surfacing as a validation error from this rule.POSTisn't covered here (as written it couldn't be -Find(0)returns null for a not-yet-created list) - ImportList minRefreshInterval cannot be changed via the API (PUT is silently ignored) #23 is specifically aboutPUT, andPOSTalready discards a client-supplied value the same way it always has.Test plan
ImportListControllerMinRefreshIntervalFixture(4 cases: reject a real change, allow leaving it unchanged, allow omitting the field, defer to normal not-found handling for an unknown id) - built against the realImportListControllerand the sameValidateResourcemethod the ASP.NET pipeline calls on every PUT