Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qualify columns with table name in generated SQL #4487

Merged
merged 2 commits into from
May 1, 2024

Conversation

mattwiller
Copy link
Member

Fixes #4129

@mattwiller mattwiller requested a review from a team as a code owner April 30, 2024 21:26
@mattwiller mattwiller self-assigned this Apr 30, 2024
Copy link

vercel bot commented Apr 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medplum-provider 🛑 Canceled (Inspect) Apr 30, 2024 9:51pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
medplum-app ⬜️ Ignored (Inspect) Visit Preview Apr 30, 2024 9:51pm
medplum-storybook ⬜️ Ignored (Inspect) Visit Preview Apr 30, 2024 9:51pm
medplum-www ⬜️ Ignored (Inspect) Visit Preview Apr 30, 2024 9:51pm

@mattwiller mattwiller added the search Features and fixes related to search label Apr 30, 2024
@mattwiller mattwiller added this to the April 30th, 2024 milestone Apr 30, 2024
Copy link

sonarcloud bot commented Apr 30, 2024

Copy link
Member

@codyebberson codyebberson left a comment

Choose a reason for hiding this comment

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

lgtm 👍

2 questions on undefined table name. I'm sure there was good reasons, just not obvious to me.

operator: Operator,
values: string[]
): Expression {
const column = new Column(details.array ? undefined : table, details.columnName);
Copy link
Member

Choose a reason for hiding this comment

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

undefined? Curious about why that was necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is basically a workaround carried forward from the existing implementation, which requires it to deal with the ArraySubquery where the inner subquery needs to refer to the unqualified column name (aliased from unnest())

} else {
const values = filter.value
.split(',')
.map((v) => new Condition(details.columnName, fhirOperatorToSqlOperator(filter.operator), v));
.map(
(v) => new Condition(new Column(undefined, details.columnName), fhirOperatorToSqlOperator(filter.operator), v)
Copy link
Member

Choose a reason for hiding this comment

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

undefined?

@mattwiller mattwiller added this pull request to the merge queue May 1, 2024
Merged via the queue into main with commit 904bf23 May 1, 2024
32 checks passed
@mattwiller mattwiller deleted the qualified-sql-columns branch May 1, 2024 17:14
medplumbot added a commit that referenced this pull request May 25, 2024
Fix age display in PatientSummary (#4484)Fixes #4471 - DetectedIssue.status valueset and search param (#4483)
Qualify columns with table name in generated SQL (#4487)
fix(migrations): only take lock if migrating (#4490)
Document updating profiles (#4402)
Run expand tests against old and new (#4503)
Adding OpenCareHub support post (#4504)
[Medplum Provider app] Various fixes and touchups (#4500)
ci(agent): add workflow for building agent outside of a release (#4512)
feat(agent): `Agent/$reload-config` operation (#4457)
Remove broken links to Foo Provider (#4518)
Dependency upgrades 2024-05-06 (#4515)
PatientSummary and provider app tweaks (#4521)
Fixes #4509 - Improve exact match search support for token parameters in `matchesTokenFilter` (#4516)
Link to new Demo Applications (#4514)
Clarify that autobatching only applies to `GET` requests (#4479)
Added missing useEffect dependency in chat demo (#4527)
fix: allows CORS for `keyvalue` API (#4476)
Fixes #4508 - MeasureReport-subject search param backport (#4530)
Fix CLI update-server version flag (#4534)
Patient summary appointments and encounters links (#4524)
Remove unused DB columns (#4532)
fix(agent): unwrap response for `$reload-config` by id (#4542)
`PatientSummary` Problem List uses US Core profile (#4535)
fix(cli): always exit with exit code 1 after error occurs during command (#4536)
Add failing test about validating nested extensions (#4548)
Add error message when `cli` fails on login (#4507)
Remove functions moved to core (#4547)
fix: AttachmentDisplay use uncached url for download link (#4501)
feat(agent): respect `Agent.status` and `Agent.channel.endpoint.status` being `off` (#4523)
CMS 1500 and Superbill (#4543)
Demo Bot: Agent Setup (#4555)
feat(Subscription): add `author` as a `SearchParameter` (#4540)
Dependency upgrades 2024-05-13 (#4544)
Full linked Project ordering in CodeSystem lookup (#4522)
Disable super admin refresh tokens (#4492)
Minor fixes for the agent setup bot (#4560)
docs(agent): document how logging works with `Bot` and `Agent` (#4563)
Split rate limits into two buckets (#4568)
Properly detect array elements (#4569)
Apply filter to ValueSet with expansion.contains (#4570)
More efficiently validate included concepts (#4573)
Dependency upgrades 2024-05-20 (#4574)
tweak(agent): add timezone in status `lastUpdated` time (#4564)
fix(client/keyvalue): set keyvalue content-type text (#4575)
Allow configuring server default rate limits (#4491)
feat(cli): add `token` command to get access token (#4579)
Updating device resources and videos (#4578)
fix(subscriptions): don't retry ws subs if sub is deleted (#4577)
Add support for 'pr' filter operation (#4584)
Super admin endpoint for database stats (#4443)
github-merge-queue bot pushed a commit that referenced this pull request May 25, 2024
Fix age display in PatientSummary (#4484)Fixes #4471 - DetectedIssue.status valueset and search param (#4483)
Qualify columns with table name in generated SQL (#4487)
fix(migrations): only take lock if migrating (#4490)
Document updating profiles (#4402)
Run expand tests against old and new (#4503)
Adding OpenCareHub support post (#4504)
[Medplum Provider app] Various fixes and touchups (#4500)
ci(agent): add workflow for building agent outside of a release (#4512)
feat(agent): `Agent/$reload-config` operation (#4457)
Remove broken links to Foo Provider (#4518)
Dependency upgrades 2024-05-06 (#4515)
PatientSummary and provider app tweaks (#4521)
Fixes #4509 - Improve exact match search support for token parameters in `matchesTokenFilter` (#4516)
Link to new Demo Applications (#4514)
Clarify that autobatching only applies to `GET` requests (#4479)
Added missing useEffect dependency in chat demo (#4527)
fix: allows CORS for `keyvalue` API (#4476)
Fixes #4508 - MeasureReport-subject search param backport (#4530)
Fix CLI update-server version flag (#4534)
Patient summary appointments and encounters links (#4524)
Remove unused DB columns (#4532)
fix(agent): unwrap response for `$reload-config` by id (#4542)
`PatientSummary` Problem List uses US Core profile (#4535)
fix(cli): always exit with exit code 1 after error occurs during command (#4536)
Add failing test about validating nested extensions (#4548)
Add error message when `cli` fails on login (#4507)
Remove functions moved to core (#4547)
fix: AttachmentDisplay use uncached url for download link (#4501)
feat(agent): respect `Agent.status` and `Agent.channel.endpoint.status` being `off` (#4523)
CMS 1500 and Superbill (#4543)
Demo Bot: Agent Setup (#4555)
feat(Subscription): add `author` as a `SearchParameter` (#4540)
Dependency upgrades 2024-05-13 (#4544)
Full linked Project ordering in CodeSystem lookup (#4522)
Disable super admin refresh tokens (#4492)
Minor fixes for the agent setup bot (#4560)
docs(agent): document how logging works with `Bot` and `Agent` (#4563)
Split rate limits into two buckets (#4568)
Properly detect array elements (#4569)
Apply filter to ValueSet with expansion.contains (#4570)
More efficiently validate included concepts (#4573)
Dependency upgrades 2024-05-20 (#4574)
tweak(agent): add timezone in status `lastUpdated` time (#4564)
fix(client/keyvalue): set keyvalue content-type text (#4575)
Allow configuring server default rate limits (#4491)
feat(cli): add `token` command to get access token (#4579)
Updating device resources and videos (#4578)
fix(subscriptions): don't retry ws subs if sub is deleted (#4577)
Add support for 'pr' filter operation (#4584)
Super admin endpoint for database stats (#4443)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
search Features and fixes related to search
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

Ambiguous column error with chained search
2 participants