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

fix: handle queries on non-existing table gracefully #3869

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

taimoorzaeem
Copy link
Collaborator

@taimoorzaeem taimoorzaeem commented Jan 21, 2025

Add new TableNotFound error and add json error
message for this error. Closes #3697.

(Couldn't think of a better changelog/commit message. Feel free to suggest a new/better message).

@taimoorzaeem taimoorzaeem force-pushed the non-existing-table/issue3697 branch from 95d4973 to 52591c0 Compare January 21, 2025 14:26
@@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- #3795, Clarify `Accept: vnd.pgrst.object` error message - @steve-chavez
- #3779, Always log the schema cache load time - @steve-chavez
- #3706, Fix insert with `missing=default` uses default value of domain instead of column - @taimoorzaeem
- #3697, Handle queries on non-existing table gracefully - @taimoorzaeem
Copy link
Member

@steve-chavez steve-chavez Jan 21, 2025

Choose a reason for hiding this comment

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

Seeing the change in the tests output, I think this should also be listed in ### Changed.

This also means that we should release it on the upcoming major.

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean you would not consider backpatching this?

Personally, I don't think this needs to be explicitly mentioned. It's an error condition that nobody relies on. This only comes up when they do something wrong - and they get a better error now. So this is hardly breaking any existing workflows, right?

(maybe I missed the relevant test output change, though - I assume it's about the 400 -> 404, from no foreign key relationship -> no table)

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean you would not consider backpatching this?

Yeah, I was thinking no backpatch. Just to be extra careful.

So this is hardly breaking any existing workflows, right?

I know some users that rely on table requests with no resource embedding being fast (in presence of slow relationships loads), if that has changed (test mentioned on #3869 (comment)) then it would certainly break existing workflows.

Copy link
Member

Choose a reason for hiding this comment

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

I know some users that rely on table requests with no resource embedding being fast (in presence of slow relationships loads), if that has changed (test mentioned on #3869 (comment)) then it would certainly break existing workflows.

So the way we used to think about this was, that "hitting the database" is the slow part and "returning early with information from schema cache" was the fast path, right?

But now this logic is turned around - and schema cache is potentially slow?

I see why you would not want to backpatch this, agreed.

@steve-chavez
Copy link
Member

Now that we rely on the schema cache for simple table filters, we need to make sure these requests are fast when a schema cache load for relationships is slow (#3046).

There's a test that validates the above behavior (a request with resource embedding is blocked until the schema cache finishes loading):

def test_requests_wait_for_schema_cache_reload(defaultenv):
"requests that use the schema cache (e.g. resource embedding) wait for the schema cache to reload"
env = {
**defaultenv,
"PGRST_DB_SCHEMAS": "apflora",
"PGRST_DB_POOL": "2",
"PGRST_DB_ANON_ROLE": "postgrest_test_anonymous",
"PGRST_SERVER_TIMING_ENABLED": "true",
}
with run(env=env, wait_max_seconds=30) as postgrest:
# reload the schema cache
response = postgrest.session.get("/rpc/notify_pgrst")
assert response.status_code == 204
postgrest.wait_until_scache_starts_loading()
response = postgrest.session.get("/tpopmassn?select=*,tpop(*)")
assert response.status_code == 200
plan_dur = parse_server_timings_header(response.headers["Server-Timing"])[
"plan"
]
assert plan_dur > 10000.0

We need an additional test that asserts that a table request (with no resource embedding) finishes in a short plan_dur after reloading the schema cache (calling /rpc/notify_pgrst in the test).

test/spec/Feature/Query/DeleteSpec.hs Outdated Show resolved Hide resolved
@@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- #3795, Clarify `Accept: vnd.pgrst.object` error message - @steve-chavez
- #3779, Always log the schema cache load time - @steve-chavez
- #3706, Fix insert with `missing=default` uses default value of domain instead of column - @taimoorzaeem
- #3697, Handle queries on non-existing table gracefully - @taimoorzaeem
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean you would not consider backpatching this?

Personally, I don't think this needs to be explicitly mentioned. It's an error condition that nobody relies on. This only comes up when they do something wrong - and they get a better error now. So this is hardly breaking any existing workflows, right?

(maybe I missed the relevant test output change, though - I assume it's about the 400 -> 404, from no foreign key relationship -> no table)

@taimoorzaeem taimoorzaeem marked this pull request as draft January 22, 2025 15:16
@taimoorzaeem taimoorzaeem force-pushed the non-existing-table/issue3697 branch from f008e21 to bdaf6bf Compare January 23, 2025 08:19
@taimoorzaeem taimoorzaeem marked this pull request as ready for review January 23, 2025 08:30
@taimoorzaeem
Copy link
Collaborator Author

taimoorzaeem commented Jan 24, 2025

One last concern, we are still using NotFound error in a couple places, which returns an empty error json:

toJSON NotFound = JSON.object []

This seems fine for now, but maybe should be improved later (add error msg, details etc) for better UX?

@taimoorzaeem taimoorzaeem force-pushed the non-existing-table/issue3697 branch from a7c3d36 to a595709 Compare January 26, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Insert into non-existing table raises empty APIErrror
4 participants