fix: normalize bare hosted NAMS endpoint to /v1 (#129) - #166
Open
Navaneethp007 wants to merge 1 commit into
Open
fix: normalize bare hosted NAMS endpoint to /v1 (#129)#166Navaneethp007 wants to merge 1 commit into
Navaneethp007 wants to merge 1 commit into
Conversation
|
@Navaneethp007 is attempting to deploy a commit to the lyonwj's projects Team on Vercel. A member of the Team first needs to authorize it. |
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
A bare hosted endpoint silently spoke the wrong wire protocol.
NamsConfig(endpoint="https://memory.neo4jlabs.com")— the base URL as printed in the docs, without the/v1suffix — fails the/v<N>search indetect_protocol, which falls back to the TCK bridge protocol. The SDK then sent snake-case bridge POSTs to the REST-only hosted service and failed with no indication that the endpoint was the cause. Now normalized tohttps://memory.neo4jlabs.com/v1when the endpoint targets the hosted host and carries no version segment.Normalizing the endpoint, not
detect_protocolThe obvious fix — make
detect_protocolreturn"rest"for the hosted host — is insufficient.build_urlconstructs REST URLs asbase + rest_path, so the request would still go tohttps://memory.neo4jlabs.com/conversations/…with no/v1: right protocol, wrong path. The version segment has to live in the endpoint string itself, which fixes both the detection and the URL in one move.Scoped to the hosted host
A blanket "append
/v1when missing" would break the TCK bridge.test_localhost_no_version_is_bridgeasserts thathttp://localhost:8000resolves tobridge, and that's deliberate — the bridge protocol is what the conformance reference implementation speaks. Normalization therefore keys on the hostname; version-less on-prem andlocalhostendpoints are untouched.The
MEMORY_ENDPOINTenv alias was a second vector_resolve_backendsets the endpoint by direct assignment (self.nams.endpoint = env_endpoint), and Pydantic v2 does not run field validators on assignment unlessvalidate_assignment=True. ANamsConfigvalidator alone would have left this path broken, so the normalization is factored into a module-level helper applied at both entry points.Deferred to a follow-up
This is a targeted fix and does not address the underlying design:
transport_mode="bridge"as the explicit opt-in would cover the whole class — but that is a breaking change and a maintainer call, not something to fold into a bug fix./v1?") would generalize the fix;NamsConfig.validate_on_connectlooks like the natural hook.Happy to open a follow-up issue for any of these if the direction is welcome.
Verification
tests/unit/nams/test_settings.py: normalization (bare, trailing slash), resulting REST detection, no-ops (/v1,/v2already present), guards (localhost, other hosts), and the env alias path.ruff check+ruff format --checkclean;mypyclean on the touched module.Fixes #129.