PostgREST / postgrest

REST API for any Postgres database
https://postgrest.org
MIT License
23.36k stars 1.03k forks source link

Open API ignores pg type comment #1377

Open fstn opened 5 years ago

fstn commented 5 years ago

I declared my procedure as:

CREATE TYPE user_acquisition_search_type AS (
    min_date_range text,
    max_date_range text,
    event text,
    pixel_key text);

COMMENT ON type user_acquisition_search_type IS
    'min_date_range text,
    max_date_range text,
    event text,
    pixel_key text';

CREATE OR REPLACE FUNCTION user_acquisition(search user_acquisition_search_type)
    RETURNS JSONB AS
$$

When I check the generated open api, I get:

 "/rpc/user_acquisition": {
      "post": {
        "tags": [
          "(rpc) user_acquisition"
        ],
        "summary": " TOTAL USER ACQUISITION ",
        "description": "   ",
        "produces": [
          "application/json",
          "application/vnd.pgrst.object+json"
        ],
        "parameters": [
          {
            "required": true,
            "schema": {
              "required": [
                "search"
              ],
              "type": "object",
              "description": "   ",
              "properties": {
                "search": {
                  "format": "total_user_acquisition_search_type",
                  "type": "string"
                }
              }
            },
            "in": "body",
            "name": "args"
          },....

I should have my real search user_acquisition_search_type and the comment I applied on it into the openapi json file instead of nothing.

jsommr commented 5 years ago

The reason seems to be

https://github.com/PostgREST/postgrest/blob/713b214c9ab44cad3d3df83ed6fa663901406857/src/PostgREST/OpenAPI.hs#L40-L51

having a predefined list of supported types, where everything else is treated as a string. And also

https://github.com/PostgREST/postgrest/blob/713b214c9ab44cad3d3df83ed6fa663901406857/src/PostgREST/OpenAPI.hs#L97-L102

which doesn't support objects.

If the function is immutable, it would be accessible via a GET request, which would probably need to be modeled somehow in Swagger to match query strings like search.event=a&search.pixel_key=b (with each property marked as the appropriate type), and a model supporting POST requests, where the parameters probably would be something like { "search": { "event": "a", "pixel_key": "b", ... } }. Just guessing...

I'd assume what's in your comment should simply be infered from the type. No need for a comment with type definitions.

Regarding your title: I initially thought you meant a Postgres PROCEDURE introduced in v11. Took me a while to figure out you meant your FUNCTION argument type total_user_acquisition_search_type didn't get added correctly.

I have never written a line of Haskell in my life, so I probably can't help you though.

steve-chavez commented 5 years ago

Thanks for the help here @nerfpops. You're right in that we default to a string for unknown pg types.

@fstn I see you want to document the function arguments. As a workaround, perhaps you could COMMENT on the function instead?

fstn commented 5 years ago

Thanks this is what I finally done ;) Stephen ZAMBAUX

0680114469

[image: Mailtrack] https://mailtrack.io?utm_source=gmail&utm_medium=signature&utm_campaign=signaturevirality5& Sender notified by Mailtrack https://mailtrack.io?utm_source=gmail&utm_medium=signature&utm_campaign=signaturevirality5& 27/08/19 à 15:39:38

Le mar. 27 août 2019 à 15:36, Steve Chávez notifications@github.com a écrit :

Thanks for the help here @nerfpops https://github.com/nerfpops. You're right in that we default to a string for unknown pg types.

@fstn https://github.com/fstn I see you want to document the function arguments. As a workaround, perhaps you could COMMENT on the function instead?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/PostgREST/postgrest/issues/1377?email_source=notifications&email_token=AAWNAGLINIPDWJUDWPUJUF3QGV64HA5CNFSM4ILPTKYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5I36KA#issuecomment-525451048, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWNAGOW5SFRZDN226LMIQDQGV64HANCNFSM4ILPTKYA .

jsommr commented 5 years ago

Hi @steve-chavez, thank you so much for all the work you put into this project. I see that you renamed the issue, but shouldn't postgrest ideally spit out a schema matching the function signature, even with composite types?

jsommr commented 5 years ago

I thought this would be a simple starter issue for learning Haskell, but it seems to not be that easy after all.

I'm trying to figure out how to add type comments in openapi, but to do so, I'm imagining something like the following will have to be done...

Currently, these values are preloaded: https://github.com/PostgREST/postgrest/blob/713b214c9ab44cad3d3df83ed6fa663901406857/src/PostgREST/DbStructure.hs#L58-L66

wouldn't it make sense to preload all the types as well, with their comments?

PgType would need to contain all the required information for a type, and no longer just a QualifiedIdentifier https://github.com/PostgREST/postgrest/blob/713b214c9ab44cad3d3df83ed6fa663901406857/src/PostgREST/Types.hs#L88

My noob skills in Haskell is going to show it's ugly face here, but to enable us to print comments, and also the type information, PgType would need to look something like this:

data PgTypeDescription = PgTypeDescription {
    ptdSchema :: Text
  , ptdName :: Text
  , ptdDescription :: Maybe Text
} deriving (Eq, Show, Ord)

-- Base type name in Postgres (text/jsonb/uuid/...)
data PgBaseType = Text deriving (Eq, Show, Ord)

data PgType
  = Scalar PgTypeDescription PgBaseType
  | Composite PgTypeDescription [PgType]
  deriving (Eq, Show, Ord)

With [PgType] added to Composite because you can't create composite types without columns.

When preloading types, it must be each and every type in the database, since you can reference any type outside the public schema.

Would this be a viable approach?

Edit: On second thought, I'll probably get in trouble with circular references, if I add [PgType] to Composite. I should stick with

data PgTypeDescription = PgTypeDescription {
    ptdSchema :: Text
  , ptdName :: Text
  , ptdDescription :: Maybe Text
} deriving (Eq, Show, Ord)

data PgType
  = Scalar PgTypeDescription
  | Composite PgTypeDescription
  deriving (Eq, Show, Ord)
steve-chavez commented 5 years ago

@nerfpops Seems like a solid approach.

You'd need to query pg_type first and then convert to Haskell types. A type can be created on a particular schema. At first I was thinking we should only get the exposed db-schema types. But getting all of the db types seems fine.

As a first step it'd be better to focus on getting only the types comments. A nice mapping between the pg type and haskell type could get cumbersome as you mentioned. This could be refined later.