fix: make a Datasphere describe that cannot answer fail instead of answering - #248
Merged
Merged
Conversation
…swering Closes #244. LoadResourceDetails turned every failure into the row {"error", "<message>"}. That row was then resize(15)'d and emitted, so SELECT name FROM datasphere_describe_asset('SP', 'MISSING') answered with the literal string "error" as the asset's name. A caller could not tell a missing asset from an asset called "error", nor an unreachable tenant from an empty one. Nothing in the result indicated anything had gone wrong - a wrong answer rather than an error. The root was an outer catch-all that swallowed everything, including the explicit throws added in this same change. It propagates now, after tracing what failed. The individual error-row sites throw instead of fabricating a row, naming the resource and the space. The reported disagreement is settled, and both readings were partly right. Reading the code decides it: the extended-metadata block resize(15)s before the scan inspects the row, so the ROW wins rather than a STRUCT cast error. But the cast concern was real too and latent - resize() fills with a default-constructed Value, an untyped NULL, and column 10 is declared as a STRUCT. Only the CATCH branch filled it with MakeEmptyAnalyticalSchemaValue(); the success branch did not. Same column, guarded in one branch of two. Both now fill it. The scan's two fallbacks are gone. One wrote the same VARCHAR into all 15 columns including the STRUCT at index 10; the other reported "Error: No space found" as the space's NAME. Both are unreachable now that the load throws, and both are hard failures rather than shapes that write strings into typed columns. Removing the catch-all immediately exposed an incomplete fix of my own from #243: the describe binds parsed `secret :=` and used it for their own token fetch, but never assigned it to the bind data - so LoadResourceDetails, which fetches a token of its own, kept using the "datasphere" default. The test asserting the name was threaded had been passing because the resulting failure was swallowed. Assigned now, and that test red-checks properly. Tested against a tenant that does not resolve, which needs no real credentials: the catalog URLs are built as https://<tenant>.<data_center>.hcs.cloud.sap/... with no loopback hatch, so a local server cannot stand in for one. The fixture carries a pre-valid token so resolution never reaches RefreshTokens and cannot open a browser. Red-checked: restoring the catch-all fails the describe cases, and removing the secret_name assignment fails the threading case.
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.
Closes #244.
The defect
LoadResourceDetailsturned every failure into the row{"error", "<message>"}, which was thenresize(15)'d and emitted. So:answered with the literal string
"error"as the asset's name. A caller could not tell a missing asset from an asset callederror, nor an unreachable tenant from an empty one — and nothing in the result said anything had gone wrong.The reported disagreement, settled
The two reviewers disagreed about whether a missing asset throws on a STRUCT cast or emits an error row. Reading the code decides it, and both were partly right:
resize(15)s before the scan inspects the row, so the row wins — no cast error.resize()fills with a default-constructedValue(untyped NULL), and column 10 is declared a STRUCT. Only the catch branch filled it withMakeEmptyAnalyticalSchemaValue(); the success branch didn't. Same column, guarded in one branch of two.Both now fill it, and the root cause — an outer
catch (const std::exception&)that swallowed everything, including the explicit throws added here — propagates instead.The scan's two fallbacks are gone: one wrote the same VARCHAR into all 15 columns including the STRUCT, the other reported
"Error: No space found"as the space's name.What removing the catch-all exposed
An incomplete fix of my own from #243. The describe binds parsed
secret :=and used it for their own token fetch, but never assigned it to the bind data — soLoadResourceDetails, which fetches a token of its own, kept using the"datasphere"default.The test asserting that threading had been passing because the resulting failure was swallowed. Assigned now, and that test red-checks properly. Worth stating plainly: a swallowing catch had been hiding a bug in the fix for a different bug.
Testing
Against a tenant that does not resolve, which needs no real credentials — the catalog URLs are built as
https://<tenant>.<data_center>.hcs.cloud.sap/...with no loopback hatch, so a local server can't stand in. The fixture carries a pre-valid token so resolution never reachesRefreshTokensand cannot open a browser.secret_nameassignment fails the threading caseNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.