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

Refactors and fixes for schema cache #3644

Merged
merged 15 commits into from
Jul 9, 2024

Conversation

wolfgangwalther
Copy link
Member

A collection of fixes and refactors for the schema cache that just happened while I was staring at this file...

Comment on lines -12 to -16
nonExistentSchema :: SpecWith ((), Application)
nonExistentSchema = do
describe "Non existent api schema" $ do
it "succeeds when requesting root path" $
get "/" `shouldRespondWith` 200
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand why we even have tests like this. Was this wanted behavior? Or was this just what happened with our code and so we observed it with tests somehow?

When I enter a wrong schema for db-schemas, I really want PostgREST to fail.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I don't remember this. I think failure is better too.

Copy link
Member

@steve-chavez steve-chavez Jul 8, 2024

Choose a reason for hiding this comment

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

Btw, is this a feat or a fix? I was thinking we could release a new minor version with this PR.

Copy link
Member

Choose a reason for hiding this comment

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

On that matter, do any of the simplified queries rely on pg > 11? If so, then we would have to do a major version for the fixes anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, is this a feat or a fix?

Given that this was explicitly tested for, I don't think we can reasonably classify this as a bug and fix.

On that matter, do any of the simplified queries rely on pg > 11?

I don't think anything in here depends on PG 12+ - but the changes made while removing the conditionals for previous versions cause merge conflicts. I'm not keen on solving those conflicts for all the refactor commits here.

We can apply the first 3 fixes on v12 easily, but the feat needs those other commits, so I don't plan on backporting that, even if we labeled it a fix.

I was thinking we could release a new minor version with this PR.

We don't backport minor versions. I assume you meant a new patch version.

For a patch version, I wouldn't want to backport all those refactors, because that could break existing stuff in theory. We should really only backport fixes, not more.

@wolfgangwalther
Copy link
Member Author

While I was doing those refactors, I was running postgrest-run --dump-schema in watch mode, temporarily commited the changes and observed all changes. The refactors don't cause any (except for ordering things in one of the commits).

Copy link
Member

@steve-chavez steve-chavez left a comment

Choose a reason for hiding this comment

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

Great work! 🎉

This needs to be loaded from in-database configuration first, otherwise
the dump-schema output will be for the default (public) schema.
There is no reason to hide those, right?
There is already a GROUP BY in the same SELECT.
Those conditions are covered by the respective nspname = ANY branches.
Casting pg_catalog to regnamespace is slightly more efficient, because
the comparison will be oid-based, not text-based.
This helps diffing schema cache changes during development.
Less joins are much easier to read and understand.
… present

The schema cache and OpenAPI output would currently list the first found
enum with the same name instead of the correct type. One other case
where this comes up is when a regular type and an enum type have the
same name. For example in the spec fixtures, we have an enum called
"bit". Every "bit" type, no matter whether it's that enum or the
built-in bit type, will show those enum options in the OpenApi output.

Not adding a test, because OpenAPI is supposed to go away in the future
anyway.
This allows to re-use ANY($$1) in the next commit.
Previously, we'd silently report "200 OK" on the root endpoint, but
would never return any endpoints from the schema cache.

Now the schema cache query fails because of the ::regnamespace cast.
@wolfgangwalther wolfgangwalther merged commit b598b59 into PostgREST:main Jul 9, 2024
0 of 2 checks passed
@wolfgangwalther wolfgangwalther mentioned this pull request Jul 9, 2024
@wolfgangwalther wolfgangwalther deleted the refactor-schema-cache branch September 1, 2024 13:11
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.

3 participants