Skip to content

fix(ENGKNOW-3566): fail loud on MDR file lookup resolution failure - #132

Merged
gmagnu merged 6 commits into
mainfrom
ENGKNOW-3566-gor-mdr-file-lookup-seem-to-silently-fail-without-error
Jul 24, 2026
Merged

fix(ENGKNOW-3566): fail loud on MDR file lookup resolution failure#132
gmagnu merged 6 commits into
mainfrom
ENGKNOW-3566-gor-mdr-file-lookup-seem-to-silently-fail-without-error

Conversation

@gmagnu

@gmagnu gmagnu commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Problem

ENGKNOW-3566 — a .gord dictionary lookup over MDR-backed sources could silently fail. A single unresolvable MDR document yielded a degenerate zero-row header (the raw mdr://…cram?env=prd URL as a data value) instead of an error; with two tags it surfaced only as a confusing "header mismatch" error.

gor source/bam/bam.gord -f A,B | top 10   # header mismatch error
gor source/bam/bam.gord -f A   | top 10   # works
gor source/bam/bam.gord -f B   | top 10   # bogus header, zero rows, NO error

Root cause

Three silent-failure holes in MdrServer.java:

  1. cacheMdrUrlscatch (Throwable e) {} swallowed every error (auth, outage, bad response), nothing logged.
  2. cacheMdrUrlss.file = u.url() assigned even when the resolved url was null/blank, corrupting the source into a garbage path.
  3. getMdrDocument/resolveMdrUrl — never checked the resolved url was non-blank, so a blank url flowed downstream and was opened as a bogus file.

Fix — fail loud

Invariant: an mdr:// source resolves to a real, non-blank URL, or the query throws a clear GorResourceException. No blank/unresolved url reaches a reader, no MDR error is swallowed without at least a log record.

  • New pure, network-free validateResolved(MdrUrlsResult, URI) — throws on null result, urls().size() != 1, or null/blank url. getMdrDocument routes through it.
  • resolveMdrUrl guards its return against a blank cached url.
  • cacheMdrUrls skips caching/rewrite on a blank resolved url (leaves mdr:// for per-source resolution) and logs bulk-cache failures at WARN instead of swallowing. Bulk caching stays best-effort by design — per-source read now fails loud.

Scope confined to MdrServer.java. Exact stg-side reason a document resolves blank is a separate deferred root-cause follow-up.

Tests

New plain unit test UTestMdrServer (no Keycloak/network, not @Ignored): 7 cases covering valid url, null/blank url (regression), null result, null urls list, zero/multiple urls. Full :model:test — 1488 pass, no regression.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings July 23, 2026 18:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens MDR-backed mdr:// source resolution in the model driver layer so MDR lookup failures can’t silently degrade into bogus/blank resolved paths; instead, resolution now consistently throws a clear GorResourceException. This aligns MDR resolution behavior with the expectation that dictionary lookups either resolve to a real URL or fail loudly at read time.

Changes:

  • Add MdrServer.validateResolved(...) to enforce “exactly one, non-blank URL” invariants for MDR responses, and route per-document resolution through it.
  • Harden resolveMdrUrl and cacheMdrUrls to avoid propagating/caching blank URLs and to warn-log bulk-cache failures instead of swallowing them.
  • Add UTestMdrServer unit tests covering null/blank URL and malformed MDR response shapes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
model/src/main/java/org/gorpipe/gor/driver/providers/stream/sources/mdr/MdrServer.java Enforces non-blank resolved MDR URLs, logs bulk resolution failures, and prevents blank URL caching/rewrite.
model/src/test/java/org/gorpipe/gor/driver/providers/mdr/UTestMdrServer.java Adds focused unit tests for validateResolved invariants and regression cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

Junit Tests - Summary

4 747 tests  +11   4 569 ✅ +11   18m 58s ⏱️ -20s
  489 suites + 1     178 💤 ± 0 
  489 files   + 1       0 ❌ ± 0 

Results for commit 7dd28d6. ± Comparison against base commit fc81c83.

♻️ This comment has been updated with latest results.

Copilot AI review requested due to automatic review settings July 23, 2026 18:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

resolveMdrUrl derived the cache read key from the mdr:// query string
(hardcoded direct fallback) but wrote under the server-echoed url_type.
With gor.mdr.mdrDefaultLinkType=presigned this wrote under PRESIGNED and
read under DIRECT, missing the cache on every resolve and re-hitting MDR.

- Add resolveUrlType(URI): derives url_type purely from the request
  (uri + config default) so read and write keys always match.
- getMdrDocument returns the validated MdrUrlsResultItem directly,
  removing the duplicated urls().get(0) extraction and dead return value.
- cacheMdrUrls guards mdrResult.urls()==null so a null-urls bulk response
  fails loud instead of NPE-ing into the best-effort catch.
- Add regression tests for resolveUrlType key derivation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 24, 2026 12:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

@gmagnu
gmagnu merged commit e8f45c1 into main Jul 24, 2026
15 checks passed
@gmagnu
gmagnu deleted the ENGKNOW-3566-gor-mdr-file-lookup-seem-to-silently-fail-without-error branch July 24, 2026 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants