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

--dump-schema does not dump media handlers #3237

Closed
wolfgangwalther opened this issue Feb 19, 2024 · 3 comments · Fixed by #3238
Closed

--dump-schema does not dump media handlers #3237

wolfgangwalther opened this issue Feb 19, 2024 · 3 comments · Fixed by #3238
Labels

Comments

@wolfgangwalther
Copy link
Member

wolfgangwalther commented Feb 19, 2024

Environment

  • PostgreSQL version: 16
  • PostgREST version: 12.1 (development)
  • Operating system: Linux

Description of issue

While trying to debug another possible regression in the current development version, I wanted to see the media handlers via --dump-schema.

(Edit: The following commands only work after 4a796e0)

Running postgrest-with-postgresql-16 -f test/spec/fixtures/load.sql postgrest-run responds with:

...
Schema cache loaded 264 Relations, 221 Relationships, 141 Functions, 15 Domain Representations, 45 Media Type Handlers

Nice!

But, postgrest-with-postgresql-16 -f test/spec/fixtures/load.sql postgrest-run --dump-schema | jq '.dbMediaHandlers' then gives me:

[]

Something is broken there.

In fact, counting the array items of the json response:

{
  "dbMediaHandlers": 0,
  "dbRelationships": 221,
  "dbRepresentations": 15,
  "dbRoutines": 141,
  "dbTables": 264,
  "dbTimezones": 0
}

Not sure if timezones is broken as well?

@wolfgangwalther
Copy link
Member Author

This seems to be the case ever since 82b3834 introduced this line of code:

    , "dbMediaHandlers"   .= JSON.emptyArray

We have the same for timezones:

https://github.com/PostgREST/postgrest/blob/4a796e0758ff1a0bd2795a6bcffa6699e55ae43b/src/PostgREST/SchemaCache.hs#L94C1-L95C45

Why?

wolfgangwalther added a commit to wolfgangwalther/postgrest that referenced this issue Feb 19, 2024
Those were left out of the schema dump when the features were introduced, probably
because ByteString doesn't have a toJSON instance. Changing the type to Text solves
this easily.

Resolves PostgREST#3237
@steve-chavez
Copy link
Member

, "dbMediaHandlers"   .= JSON.emptyArray

Ah, my bad. I forgot to mark that as TODO. It should be filled.

We have the same for timezones:

I recall this one was on purpose, to not expose it via OpenAPI. But on second thought, would it makes sense to expose it? I guess we could list all Prefer: timezone=xx/yy values but it would be a long list.. not sure.

@wolfgangwalther
Copy link
Member Author

I recall this one was on purpose, to not expose it via OpenAPI. But on second thought, would it makes sense to expose it? I guess we could list all Prefer: timezone=xx/yy values but it would be a long list.. not sure.

This is only about dumping the schema cache, not about openapi. #3238 dumps it now, too - but that doesn't mean it needs to go anywhere else, right?

wolfgangwalther added a commit to wolfgangwalther/postgrest that referenced this issue Feb 19, 2024
Those were left out of the schema dump when the features were introduced, probably
because ByteString doesn't have a toJSON instance. Changing the type to Text solves
this easily.

Resolves PostgREST#3237
wolfgangwalther added a commit to wolfgangwalther/postgrest that referenced this issue Feb 19, 2024
Those were left out of the schema dump when the features were introduced, probably
because ByteString doesn't have a toJSON instance. Changing the type to Text solves
this easily.

Resolves PostgREST#3237
wolfgangwalther added a commit that referenced this issue Feb 20, 2024
Those were left out of the schema dump when the features were introduced, probably
because ByteString doesn't have a toJSON instance. Changing the type to Text solves
this easily.

Resolves #3237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants