sdk: raise NovemException on API errors instead of a print placeholder#236
Draft
bjornars wants to merge 1 commit into
Draft
sdk: raise NovemException on API errors instead of a print placeholder#236bjornars wants to merge 1 commit into
bjornars wants to merge 1 commit into
Conversation
Every resource write path (vis/group/job/repo `create_*` PUT and `api_write` POST) handled 404/403/409 but, on any other non-ok response, printed the body + headers + the literal "should raise a general error" and returned — so the caller got no exception and silently no-op'd. Terje hit exactly this (novem-code/gaia#2656): `mail.bcc = [over-cap list]` returned without error while nothing was stored. The server now rejects over-cap / unresolvable writes with a 400 + `{message, rejected:[…]}`, but the SDK swallowed it. Add `raise_on_response(r)` (api_ref) — parses the server `message` and any `rejected` lines and raises the matching NovemException subclass (401/403/ 404/409 → generic NovemException otherwise) — and call it from all eight write sites. `mail.bcc = …` over the cap now raises NovemException with the cap reason and the rejected addresses.
Contributor
Author
|
@sondove I know there are concerns here. Maybe find a way to opt in to hard-failures, vs the default way of just |
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.
Draft — implements the fail-hard option from #235 for discussion.
What
Adds
raise_on_response(r)(api_ref) — parses the servermessageand anyrejectedlines and raises the matchingNovemExceptionsubclass (401/403/404/409, genericNovemExceptionotherwise) — and replaces theprint("should raise a general error")placeholder in all eight write sites (vis/group/job/repo, thecreate_*PUT +api_writePOST). Keeps the intentional 409-on-create no-op carve-out.Why draft
This is the fail-hard direction from #235 — a behavioral change: writes that previously silently no-op'd on a non-2xx now raise. The open questions there should be settled before this goes ready:
raise_on_error/strictflag vs hard default (existing users)Verified
Against a live dev API,
mail.bcc = [30 addrs over the 25 cap]now raises instead of silently storing nothing:The server-side 400 that makes this observable is
novem-code/gaia#2810.Before ready
rejectedbody → raises with the reasonRefs #235 · SDK half of
novem-code/gaia#2656