[APIPUB-108] Take the API's served paths from its Discovery document - #174
Open
analiliagutierrez wants to merge 9 commits into
Open
analiliagutierrez wants to merge 9 commits into
analiliagutierrez wants to merge 9 commits into
Conversation
The data management and change queries paths were fixed at the values an ODS/API uses, so a DMS, which serves data management under /data, could only be published to from behind a proxy that rewrote every request. Both are now read from the Discovery document, held relative to the connection URL so an address carrying a tenant prefix states it once, and falling back to the conventional values for an API that does not declare them. An operator can state either outright for an API whose document cannot be reached.
An API that qualifies its routes answers the unqualified address with the placeholders rather than with values, and the dependencies URL is read from that same document before any segment is resolved. Requesting one drew a 404 naming a URL with '%7B' in it, because Uri escapes the braces, which reads as a defect in the tool rather than as an API asked at the wrong address. The check that already refused such a path for the data management and change queries segments now covers this one too, so the failure names its cause and the address the connection needs.
Test Results591 tests 591 ✅ 27s ⏱️ Results for commit a87b82c. ♻️ This comment has been updated with latest results. |
…mming it A value taken from a Discovery document decided where this connection's requests went, and trimming its leading slash could turn it into an absolute URL of its own. An HttpClient follows such a value in place of its base address, so a declaration of '/https://elsewhere.example/collect' sent the connection's bearer token and the documents being published to that host. The path is now resolved with Uri against the connection's address and refused unless it lands on the connection's own host. Resolving structurally also settles the school year. A year-specific ODS/API declares a year of its own, and appending the connection's year to that addressed a year within a year; the connection's year now replaces whatever the API stated there, including an unresolved token.
… is used A refused path threw InvalidOperationException, which the exit code table reports as 2, documented as "Re-run". Re-running never resolves it. Both refusals now throw InvalidConfigurationException, so the run exits 4 and the console says the configuration is what needs correcting. The resolved path was logged at Debug, so a default run kept no record of where requests were actually sent, which is the first thing needed when an API serves somewhere other than where the tool used to assume. It is now reported at Information. Two tests expected a value that the conventional fallback produces anyway, so neither could tell resolution from fallback; both now use paths no fallback yields. Nothing covered a Discovery document that cannot be read or that carries a urls value which is not an object.
…ot read Resolution happened on first use, inside a Lazy, on whichever publishing thread got there first, so a path that could not be resolved surfaced as a block fault once the source had already been streamed. It now happens while the connection is being set up, beside the bearer token and for the same reason. The document is read through the transport rather than through the client's own pipeline. It is anonymous, so there is no reason to stamp a bearer token on the request for it, and reading it must not take a slot from the cap on concurrent requests or leave the client's HttpClient started before its caller has configured it. It is not read at all when the connection already states every path that would be taken from it. An API that answered without naming a path and one that could not be asked were reported alike, as the API not declaring it. They are now distinct: the first is ordinary, since an ODS/API declares change queries only while that feature is enabled, and is reported at Information; only the second warns. A path declared at the connection URL itself resolved to an empty segment, which made the composed resource path absolute and dropped the prefix the connection URL carries. Messages name the setting as it is written in configuration and on the command line rather than the bare property name.
…onnection one way EdFiApiConstants described its values as the path to the data management API. They are the value to assume when an API does not say where it serves, and a DMS says something else. The names also collide with the resolved segments on EdFiApiClient, so a contributor composing a request from the constant would quietly reintroduce what this ticket fixes; the remarks now say which to use. A connection appeared as Name in one class and ConnectionName in another, so no single property gathered one run's routing story. The dependencies URL is kept absolute while the path segments are held relative, which reads as an inconsistency until you know a request URI and a concatenated segment resolve differently. That is now written down. A test asserted that a refusal mentions a tenant, which the guidance appended to every refusal says regardless, so the assertion could not fail. It now asserts the escaped placeholder the rejected value actually carries. Logging is configured for the fixture rather than from inside two of its tests, which replaced the logger the whole assembly shares part-way through a run.
There was a problem hiding this comment.
🟡 Changes recommended
Tenant placeholders can be misrouted, and explicit overrides do not yet support gateways with inaccessible Discovery roots.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Enables API Publisher to discover API route paths dynamically, supporting DMS without reverse-proxy rewriting.
Changes:
- Resolves data and change-query paths from Discovery documents with configurable fallbacks.
- Validates hosts, placeholders, and school-year routing.
- Adds configuration documentation and automated coverage.
File summaries
| File | Description |
|---|---|
| src/EdFi.Tools.ApiPublisher.Tests/Processing/SourceRequestConcurrencyCapTests.cs | Preserves concurrency request-count expectations. |
| src/EdFi.Tools.ApiPublisher.Tests/Processing/EdFiClientProviderTests.cs | Tests placeholder rejection for metadata URLs. |
| src/EdFi.Tools.ApiPublisher.Tests/Processing/EdFiApiUrlSegmentResolutionTests.cs | Covers route resolution and fallback behavior. |
| src/EdFi.Tools.ApiPublisher.Tests/MockRequests.cs | Adds Discovery URL data to API mocks. |
| src/EdFi.Tools.ApiPublisher.Tests/Configuration/ConfigurationBuilderFactoryUrlSegmentTests.cs | Tests new command-line mappings. |
| src/EdFi.Tools.ApiPublisher.Core/Processing/EdFiApiConstants.cs | Clarifies conventional route constants as fallbacks. |
| src/EdFi.Tools.ApiPublisher.Core/Configuration/ConfigurationBuilderFactory.cs | Maps four route override switches. |
| src/EdFi.Tools.ApiPublisher.Connections.Api/Configuration/Enhancers/EdFiApiConnectionsConfigurationBuilderEnhancer.cs | Loads route overrides from named connections. |
| src/EdFi.Tools.ApiPublisher.Connections.Api/Configuration/ApiConnectionDetails.cs | Defines route override properties. |
| src/EdFi.Tools.ApiPublisher.Connections.Api/ApiClientManagement/IEdFiApiClientProviderExtensions.cs | Validates metadata route placeholders. |
| src/EdFi.Tools.ApiPublisher.Connections.Api/ApiClientManagement/EdFiApiUrlSegmentResolver.cs | Implements secure route resolution and normalization. |
| src/EdFi.Tools.ApiPublisher.Connections.Api/ApiClientManagement/EdFiApiClient.cs | Reads Discovery metadata during client setup. |
| docs/API-Publisher-Configuration.md | Documents routing behavior and settings. |
| docs/API-Connection-Management.md | Documents named-connection route keys. |
Review details
Suppressed comments (1)
src/EdFi.Tools.ApiPublisher.Connections.Api/ApiClientManagement/EdFiApiUrlSegmentResolver.cs:108
- The routing messages log the segment before
Finishapplies or replaces the configured school year. For example, a declareddata/2025is reported even when requests usedata/2024; the stated and conventional branches have the same mismatch. Compute the finished segment before logging in each branch so the new startup diagnostic reports the path actually used.
_logger.Information(
"The {ConnectionName:l} API declares {DiscoveryUrlName:l} as {DeclaredUrl}; requests will use '{Segment:l}' relative to '{BaseAddress}'.",
_connectionName,
definition.DiscoveryUrlName,
declaredUrl,
declaredSegment,
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… that is not one
Applying the connection's school year to any trailing route placeholder turned
a path that cannot be served into one that looks valid: a declared
'data/{tenant}' became 'data/2024' and passed the refusal that exists to catch
it, addressing the wrong route. Only the placeholder that names the school
year is a question the connection can answer; every other one stays a
configuration error.
A declared value carrying a query string or a fragment survived into the
segment, because a relative URI keeps both. A resource path is appended to a
segment rather than merged with it, so 'data?tenant=1' would have addressed
the resource inside the query string, and a fragment would have kept it from
being sent at all. Both are now refused.
The same document carries an API's version information, the URL of its dependency metadata and the paths it serves, and each of those asked for it separately: the client while connecting, the version check, and the dependency metadata provider, which builds a version provider of its own. Three requests per connection for one document, answered three times and parsed three ways. The version metadata provider now returns the copy the client read. Measured against a live DMS, one run of a publish makes one request to the target's root where it previously made several. The client no longer skips that read when both paths are stated on the connection. The skip saved nothing, because the version check needs the document on every run regardless, and it left the reuse with nothing to return. Version information is also no longer interpolated into its own log template, where a value such as a route placeholder in the document would be parsed as a property token.
…cument The settings were documented, and reported in the log, as the remedy for an API whose Discovery document cannot be reached. They are not. The publisher takes each API's version information from that same document, and the version check runs before these paths are used, so a run against an API that does not serve the document ends there whatever the connection states. What the settings do cover is an API that declares a path its callers cannot use, which is the deployment behind a gateway that declares the address it sits at rather than the one it is reached by. The documentation and the fallback warning now say that, and say what is not covered.
analiliagutierrez
marked this pull request as ready for review
September 18, 2026 18:27
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
The publisher assumed every Ed-Fi API serves its data management resources under
data/v3and its change queries underchangeQueries/v1. DMS serves data management underdata, so it could only be used as a publishing target from behind a reverse proxy that rewrote the path of every request. Both paths are now taken from the API's own Discovery document, resolved against the connection's URL, with the conventional values kept as the fallback for an API that does not declare them.Ticket
APIPUB-108 - Adjust Routing Logic for DMS Compatibility
https://edfi.atlassian.net/browse/APIPUB-108
Type of Change
What Changed
EdFiApiClientresolves both path segments while the connection is being set up, beside the bearer token and for the same reason: a connection whose requests cannot be addressed says so then, rather than from inside a processing block once the source has already been streamed.changeQueriesis resolved the same way asdataManagementApi. It carries the identical hardcoded shape, and without it DMS cannot be used as a source.DataManagementUrlSegmentandChangeQueriesUrlSegmentand the four matching command line arguments. A stated path wins and the Discovery document is not consulted for it. This covers an API that declares a path its callers cannot use, such as one behind a gateway that declares the address it sits at rather than the address it is reached by. It does not stand in for an API that serves no Discovery document at all, because the publisher takes version information from that same document and the version check runs before these paths are used.4and the console says the configuration is what needs correcting.Information, since routing now depends on what the API answers.docs/API-Publisher-Configuration.mdgains a section on where requests are sent, with a behavior-change note, and documents the four new settings.docs/API-Connection-Management.mdgains the two named-connection keys, and itsurlrow no longer describes the base URL in terms of/data/v3.Architectural Decisions
A declared path is resolved with
Uri, not by trimming strings. The value is combined with the connection's address, required to land on the connection's own host, and only then expressed relative to it. Trimming a leading slash off a value the API supplied can turn it into an absolute URL of its own, which anHttpClientfollows in place of its base address, carrying the connection's bearer token and its request bodies to whatever host the value named. Resolving structurally also means a path prefix carried by both the connection URL and the declaration is stated once, which is how a route qualified or multi tenant deployment is addressed.Precedence is stated, declared, conventional. The Discovery document alone covers both reference deployments, but it leaves no way out where an API declares a path its callers cannot reach, and the operator can see that a value is wrong while having no way to correct it. The conventional values stay in
EdFiApiConstantsas that fallback rather than being removed.The connection's school year replaces the year the API states, rather than being appended to it. A year-specific ODS/API asked for its paths at its unqualified address declares a year of its own, and some versions state an unresolved token in that position. Appending would address a year within a year. The year is applied before a path is refused for carrying a placeholder, so an API that states the year as a token is answered rather than rejected.
The placeholder check looks for the escaped spelling as well as the braces, because a placeholder survives a round trip through
Urias%7B. Only the placeholder that names the school year is answered with one; answering any other would turn a path that cannot be served into one that looks valid and addresses the wrong route.Testing
Automated
Full suite: 590 passed, 1 skipped.
Covering resolution and normalization, each precedence branch, refusal on another host and on a route placeholder, a Discovery document that is absent, unreadable, not JSON or carrying a
urlsvalue that is not an object, school year replacement including the token form, a path declared at the connection URL itself, and the four command line switch mappings including a transposition.Each test was checked against a deliberately broken implementation to confirm it can fail: disabling the Discovery lookup fails 6, disabling the placeholder detection fails 2, disabling the authority check and the school year replacement fails 5, and removing one switch mapping fails 2.
Manual
Published between live servers with no proxy in front of either.
/apipath base./ed-fi/schoolswith descriptors from the ODS into DMS.datarather thandata/v3.--targetDataManagementUrlSegment=data/v3--useChangeVersionPagingThe third run is the error path. It differs from the second only in that argument, which restores the path the tool previously assumed, and it establishes that the routing was what stood in the way.
The two servers declare different shapes and disagree on the trailing slash:
dataManagementApi{root}/data/v3/{root}/datachangeQueries{root}/changeQueries/v1/{root}/changeQueries/v1/What the run reports, at the default log level:
Published documents were confirmed by querying DMS directly rather than by reading the publisher's own summary: 6 schools, 62 state abbreviation descriptors, 1 local education agency.
A DMS configured with
ROUTE_QUALIFIER_SEGMENTS=districtIddeclares{root}/{districtId}/dataat its unqualified address. A run against that address stops with a message naming the cause and exits4, rather than requesting a URL containing%7BdistrictId%7D.Known Limitations / Follow-Up
/apipath base in every run above.--ignoreIsolation=true, and a separate story under APIPUB-107 will carry why that argument is currently required.Checklist