fix(bfabric): canonicalise base_url without a trailing slash - #596
Draft
leoschwarz wants to merge 3 commits into
Draft
fix(bfabric): canonicalise base_url without a trailing slash#596leoschwarz wants to merge 3 commits into
leoschwarz wants to merge 3 commits into
Conversation
`_validate_base_url` appended a trailing slash that essentially nothing
wanted: 16 call sites stripped it right back off to build a URL, and the
two SOAP engines forgot to, emitting `.../bfabric//workunit?wsdl` on every
`Bfabric.connect()`. The CLI's `normalize_base_url` had already settled on
the slash-free form, which is also what gets persisted to `~/.bfabricpy.yml`.
Flip the canonical form and canonicalise once at each public boundary --
the `connect_*` classmethods and `WebappClient.create` now build the
`BfabricClientConfig` first and read `base_url` back from it -- so the
downstream strips become dead rather than merely relocated. Going through
the validator is also stronger than `rstrip`: it lowercases the host, drops
a default port, and rejects a non-HTTP URL up front.
`entities/core/uri.py` had to flip alongside. It hardcoded the instance
component as `.../bfabric/`, and `EntityReader` compares that for string
equality against `config.base_url` at three sites, so changing only the
config would have made every `read_uris`/`query` raise "Unsupported
B-Fabric instance". `EntityUri` strings are unaffected -- `as_uri()`
appends its own slash before `urljoin`.
Two strips survive deliberately. `api_to_rest_url` is exported from
`bfabric.transfer` and its strip is load-bearing for the `.../api/` case,
where `endswith("/api")` is False without it. The one in
`compute_token_cache_path` is gone instead: every caller passes a
canonical value and the resulting key is byte-identical to today's, so no
cached token is orphaned -- a test now pins exactly that.
The trailing slash was originally introduced in #341 so that
`urljoin(base_url, ...)` in `Entity.web_url` would not drop the `/bfabric`
path segment. That call site has since been replaced by the `EntityUri`
machinery, and both remaining `urljoin` callers append their own slash, so
the reason no longer applies.
Closes #576
…stem The slash-free canonical form was a convention nothing checked: a caller could hand a raw string to `pkce_login` or `compute_token_cache_path` and nothing would object. That is not hypothetical -- `bfabric-cli auth register` was passing a raw `--base-url` straight into `register_client`, previously masked by a defensive `rstrip`. Add `bfabric.config.CanonicalBaseUrl`, a `str` subclass that validates and canonicalises in `__new__`, mirroring the existing `EntityUri` pattern. It stays a `str`, so interpolation, comparison and dict keys are unaffected, but annotating a parameter with it lets basedpyright reject a value that never passed through canonicalisation -- which `Annotated[str, AfterValidator(...)]` cannot, being indistinguishable from `str`. Public entry points (`connect_*`, `WebappClient.create`) keep taking `str` and canonicalise once; private helpers and the entity layer require the type. That collapses the previous construct-then-read-back dance into `base_url = CanonicalBaseUrl(base_url)`, with the config built where it belongs. `EntityUriComponents.bfabric_instance` moves from pydantic's `HttpUrl` to the same type, removing the third spelling of one concept. The `str()` coercion when building a `GroupKey` existed only because `HttpUrl` is not a `str`, and sat on exactly the seam that diverged in #576; the retype also clears a grandfathered `reportArgumentType` from the baseline. Two fixes fall out of typing the boundary: `validate_token` compared the server's `caller` against the configured `supported_bfabric_instances` by exact string equality, so a trailing slash on either side rejected a valid token. Both sides are now canonicalised. This is legacy API, so the fix is local and the settings stay `str`. Typing `normalize_base_url`'s return exposed that the CLI writes its config through unsafe `yaml.dump`, which serialised the subclass as `!!python/object/new:` -- a file the `safe_load` on read cannot parse. The writer now uses `safe_dump` (matching the reader) and the CLI writes plain strings, so a non-YAML type fails loudly instead of corrupting `~/.bfabricpy.yml`. The baseline loses two entries and gains none.
#596 fixed the canonical form of base_url and canonicalises it once per public boundary, but the resulting invariant was convention only: nothing stopped a caller handing a raw string to pkce_login or compute_token_cache_path. That was not hypothetical -- `auth register` was doing exactly that, masked by a defensive rstrip until #596 removed it. The failure modes are asymmetric. A stray slash in a URL yields `//`, which servers tolerate, which is why #576 went unnoticed for months. A stray slash in compute_token_cache_path changes the SHA-256, so the cache misses and the user is told to log in again while their token sits on disk. bfabric.BaseUrl is a str subclass that validates and canonicalises in __new__, so basedpyright rejects an unvalidated string at the boundary while every f-string, dict key and httpx call keeps working. It replaces three spellings of one concept: base_url: str, bfabric_instance: str, and EntityUriComponents.bfabric_instance: HttpUrl -- the last of which forced the str() coercion in uri.py that sat on the exact seam #576 broke. Verified the type checker actually catches it before migrating: annotating one function surfaced two real call sites, so neither the position-independent baselines nor the neighbouring inline ignores absorb the error. Two consequences worth noting: - yaml.dump would serialise a str subclass as `!!python/object/new:`, silently writing a config that no longer loads. config_writer now uses safe_dump, which rejects it loudly instead; the two CLI write paths coerce to str. - BaseUrl raises a plain ValueError rather than a pydantic ValidationError, so the CLI can print it directly. Pydantic still wraps it on a model field. Also fixes a latent bug in validate_token, which compared the server's caller against the configured instances with no canonicalisation on either side.
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
1 participant
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.
BfabricClientConfig.base_urlto be canonicalised without a trailing slash. Config files andconnect_*arguments still accept one.bfabric.BaseUrl, astrsubclass holding a validated slash-free instance URL, so an unvalidated base URL is a type error rather than a stray//or a token-cache miss. It behaves as astreverywhere, so interpolation, comparison and dict keys are unaffected.BfabricClientConfig.base_url,Entity.bfabric_instanceandEntityUriComponents.bfabric_instance(was a pydanticHttpUrl) to this type, and require it forEntityReader'sbfabric_instancearguments. ConstructingBfabricClientConfigdirectly now needsBaseUrl(...);model_validatestill accepts a plain string.connect_oauth/connect_pkce/connect_device_code/connect_patandWebappClient.createto canonicalisebase_url, so a mixed-case host or a default port is normalised too and a non-HTTP URL is rejected up front with a plainValueError.show.htmllinks printed bybfabric_readandbfabric-cli api read, to no longer contain a doubled slash.validate_tokento canonicalise both the token'scallerand the configuredsupported_bfabric_instancesbefore comparing, so a trailing slash on either side no longer rejects a valid token.bfabric-cli auth registerto canonicalise itsbase_urlargument like the otherauthcommands.bfabric-cli auth login/auth patto write the config withyaml.safe_dump, matching thesafe_loadused to read it back.bfabricPy-testsmay assert the old trailing-slash form ofbase_url.Closes #576
🤖 Prepared with assistance from Claude Opus 5 via Claude Code.