fix(trino): pin a session schema for unqualified statements - #721
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 36754982 | Triggered | Generic Password | a1cddb5 | tests/unit/seed/types.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Thanks for the patch. One check is red and it is not your code being wrong, so here is what it is and the one-line fix. GitGuardian Security Checks reports "1 secret uncovered". tests/unit/seed/types.test.ts:234 : password: "s3cret" Its generic-password detector matches password: "" without weighing entropy. Because the PR comes from a fork, GitGuardian cannot attach the finding to an incident, so there is no detail page to read behind the check, just the summary. Two things worth knowing:
Simplest fix: drop the password line from that case. The test asserts SeedConnectionSchema's schema field and the password never reaches an assertion. A placeholder such as "not-a-real-password" works too if you would rather keep the field populated. Separately, the three required checks have not run yet: fork PRs wait for a maintainer to approve workflows. I will approve them so we get a real signal on the change itself. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
The required checks have run now and they are green. The parts of this that were easy to get wrong, you got right: you found every reader of the connection field list including the two that fail nothing at compile time ( My earlier request about
Optional: Two things I am tracking rather than asking for: with the schema left empty, Create Table and the import modal's new-table path still emit unqualified statements the coordinator refuses, which #653 anticipated under Option 3; and |
|
Thanks for the detailed review. I’ve addressed all requested changes: removed the test password that triggered GitGuardian, updated the Trino session warning and matching documentation, documented the schema field in Seed Connections and the API reference, and aligned the Schema label layout and styling with its neighbors. I also added a direct unit test for the connection-pinned schema header and strengthened the warning assertion. I left the two tracked follow-ups outside this PR as noted. The full test suite, build, and coverage checks pass with 100% line coverage. |
|
All six are addressed and I checked each one rather than taking the summary. The three required checks are green. The optional test you added is the one I care about most, and it is real: setting One thing not to chase: GitGuardian will stay red on this branch. It scans commits, not the working tree, and the literal is still in your first commit. The line is gone from One cosmetic leftover I will take myself after merge: in Good work on this one. Thanks for the care on the field-list readers and on the test that asserts both arms. I will merge it shortly. |
Description
Add an optional session schema to Trino connections so unqualified statements resolve against the selected catalog and schema.
Before this change, a connection could pin only a catalog, causing statements such as
SELECT * FROM widgetsandCREATE TABLE t (...)to fail withSchema must be specified when session schema is not set.Type of Change
Related Issue
Closes #653
Changes Made
X-Trino-Schemaheader while preserving per-query overrides and the existing behavior when no schema is configured.Testing
The issue was reproduced against a live Trino 476 instance before applying the fix:
SELECT * FROM widgets_653failed because no session schema was set.CREATE TABLE created_653_baseline (...)failed for the same reason.After applying the fix with Catalog
memoryand Schemadefault:SELECTsucceeded.CREATE TABLEsucceeded./api/db/querypath returns 200 for unqualified statements when a schema is configured and the expected 400 when it is absent.Validation completed:
Test Environment
3b9a8be)Screenshots (if applicable)
Not applicable.
Checklist
Additional Notes
No dependent changes are required. The fix is compatible with existing Trino connections: an omitted or empty schema continues to send no schema header.