Update OpenAPI (SQL COMMENT to description, constraints, cleaning up) #885

ldesgoui · 2017-06-03T01:20:50Z

Greetings,

This PR addresses the idea raised in #222.
I launched the PR to ask feedback: what do you feel deserves change in the current OpenAPI generation ?

To do before completion:

Most of this seems trivial and should be accomplished soon :)

Side note:
I've updated the ToJSON instances but I don't think they're used anywhere, maybe they could get removed? (EDIT: removed now)
What about fluffing up the hard-coded OpenAPI options a little more while I'm at it ? They're not the most welcoming in the world and could definitely redirect developers to postgrest.com documentation

Links:
Postgres SQL Comments
PostgraphQL's introspection query

begriffs · 2017-06-08T05:00:55Z

You know I'm really not an OpenAPI expert. I think wherever you want the comments to appear in the output is probably a great start. Perhaps other people can chime in here. Just doing the hard part of obtaining those comments and putting them somewhere is wonderful, and it will be easier to tweak later on as people have suggestions as they use it. Having some repetition seems OK. The output can be pretty huge anyway, and I'd rather have API consumers notice the comments than overlook them.

I do like the idea of allowing a schema comment to override the generic API description placeholder. Also regarding your side note, feel free to try removing unused instances and cleaning up the code. My philosophy is that if the existing tests keep passing then we should be good.

Thanks for jumping in and working on this! I know people have wanted it for a while. Once you add some tests and a changelog entry we can look into getting this merged.

Anybody with opinions please add a comment.

…tablename.{get,post,put,delete}.description with Table description

begriffs · 2017-06-11T20:22:37Z

I learned from previous work that when the openapi test times out it is actually failing because of malformed openapi output. Not sure why it does't just return an error. In the past I copied the output into an online validator to get a useful error message.

begriffs · 2017-07-01T23:35:07Z

I know the test suite isn't very helpful when it fails for OpenAPI validation. Let's see if we can work together to figure out the problem. I checked out your branch, built it, and ran postgrest against the database schema created by our test suite. I was able to get more information about the validation failure by copying the openapi output into http://bigstickcarpet.com/swagger-parser/www/index.html :

Swagger schema validation failed. 
  Too few properties defined (0), minimum 1 at #/paths//withUnique/delete/responses
 
JSON_OBJECT_VALIDATION_FAILED

Error: Swagger schema validation failed. 
  Too few properties defined (0), minimum 1 at #/paths//withUnique/delete/responses
 
JSON_OBJECT_VALIDATION_FAILED
    at http://bigstickcarpet.com/swagger-parser/www/js/bundle.min.js:11:318
    at http://bigstickcarpet.com/swagger-parser/www/js/bundle.min.js:50:316
    at <anonymous> 

SyntaxError: Swagger schema validation failed. 
  Too few properties defined (0), minimum 1 at #/paths//withUnique/delete/responses
 
JSON_OBJECT_VALIDATION_FAILED
    at Function.syntax (http://bigstickcarpet.com/swagger-parser/dist/swagger-parser.min.js:284:318)
    at validateSchema (http://bigstickcarpet.com/swagger-parser/dist/swagger-parser.min.js:20:254)
    at http://bigstickcarpet.com/swagger-parser/dist/swagger-parser.min.js:8:1921
    at <anonymous> 

z-schema validation error: JSON_OBJECT_VALIDATION_FAILED
    at ZSchema.getLastError (http://bigstickcarpet.com/swagger-parser/dist/swagger-parser.min.js:2047:3479)
    at validateSchema (http://bigstickcarpet.com/swagger-parser/dist/swagger-parser.min.js:20:158)
    at http://bigstickcarpet.com/swagger-parser/dist/swagger-parser.min.js:8:1921
    at <anonymous>

Does this error help?

ldesgoui · 2017-07-02T07:05:50Z

Oh, that seems easy to fix, deleteOp in OpenAPI.makePathItem doesn't contain a "at" field
Thanks a lot for the help. I'll be continuing work on the PR this week, unless you'd rather discuss #790 further.

begriffs · 2017-07-02T14:48:50Z

That discussion would affect another major version anyway, and we still may keep the openapi description but use content negotiation (the Accept request header) to show it rather than show it by default. Also extracting sql comments into our in-memory data structure will be useful no matter how we output the api description so carry on!

ldesgoui · 2017-07-12T02:09:29Z

I tried using the tool you linked but I had no chance with it, when I click "Validate it!", a pretty-print of the description shows up in a box that shakes for a second and nothing else happens/shows up, where did you get those error messages?
EDIT: other validators seem to have no problem with the output, sad that the test doesn't let us know more 😦

On another note, I've been slowly reading the OpenAPI spec, it seems most of the repetition can be abstracted away using global definitions. I'll probably make the output less "precise" but smaller by removing the enums/patterns generated with columns/operators

Also: I was interested in using Vendor Extensions for FK/PK/constraints but they're not supported by hackage's swagger2 :(

ldesgoui · 2017-07-17T05:27:13Z

How the output looks after the removal of all duplication: https://gist.github.com/ldesgoui/faecf9fec35e150f67be1cb7843c1f7e

ldesgoui · 2017-07-17T05:27:39Z

Misclick, sorry 😦

begriffs · 2017-07-17T14:32:22Z

The new output looks really nice, I can actually read it directly rather than the old avalanche of text. Great work!

Did you manage to figure out the validator I was using? Looks like the tests are failing on the most recent commit except this time they are giving a real error message rather than just hanging.

ldesgoui · 2017-07-18T03:42:34Z

I did, yes, the output is valid now. The tests fail because they need to be updated to reflect changes

This reverts commit 3042754.

ldesgoui · 2017-07-19T14:55:07Z

Hey, I'm done with this PR, here's a recap:

Dead ToJSON intances were remvoed
ProcDescription, Table, Column received a new Maybe Text field for descriptions
Introspection received an additional SQL query to get the Schema COMMENT
RPC body schema was inlined as it's only used once
All Table path properties were moved to Parameter Definitions as they're all used more than once
OpenAPI links to PostgREST docs

begriffs · 2017-07-19T23:11:17Z

src/PostgREST/OpenAPI.hs

+        colDescription c
+    s =
+      (mempty :: Schema)
+        & default_ .~ (decode . TL.encodeUtf8 . TL.fromStrict =<< colDefault c)


Is it possible to use the more generic toS to convert the string types? Something like

default_ .~ (decode . toS =<< colDefault c)

Not sure if it can infer the types, but thought I'd ask.

begriffs · 2017-07-22T15:55:54Z

Thanks for all your work to research this and finish it!

ldesgoui · 2017-07-22T15:57:04Z

My pleasure, postgrest is pure joy to use in projects :)

steve-chavez · 2020-08-27T13:58:01Z

test/Feature/StructureSpec.hs

+                    "type": "string"
+                  },
+                  "parent_id": {
+                    "description": "Note:\nThis is a Foreign Key to `entities.id`.<fk table='entities' column='id'/>",


@ldesgoui I've always wondered.. Where does the <fk> and <pk> tags come from?

ldesgoui · 2020-08-27T14:00:09Z

If I remember correctly, those are basically a hack to insert metadata about what we generate, they don't "exist" and don't get through markdown generation. Probably should have looked for a better way

…

On Thu, Aug 27, 2020, 15:58 Steve Chavez ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/Feature/StructureSpec.hs <#885 (comment)>: > + { + "type": "object", + "description": "child_entities comment", + "properties": { + "id": { + "description": "child_entities id comment\n\nNote:\nThis is a Primary Key.<pk/>", + "format": "integer", + "type": "integer" + }, + "name": { + "description": "child_entities name comment", + "format": "text", + "type": "string" + }, + "parent_id": { + "description": "Note:\nThis is a Foreign Key to `entities.id`.<fk table='entities' column='id'/>", @ldesgoui <https://github.com/ldesgoui> I've always wondered.. Where does the <fk> and <pk> tags come from? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#885 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAS3VZQCMPAKW43OFN3GWXTSCZQ7VANCNFSM4DN3XBIQ> .

steve-chavez · 2020-08-27T16:41:08Z

@ldesgoui I see, thanks for the quick reply!

According to OAI/OpenAPI-Specification#587, another way of specifying the pk/fk would be with OpenAPI extensions.
This could be a future enhancement.

…PostgREST#885)

ldesgoui added 7 commits June 2, 2017 07:29

add Description to Column, Table, Procedure types

ece5993

fixed syntax error

a72662b

Match function types and fetch sentinel values from SQL

c002e41

Fix mismatches related to Column

f6f1d91

temporarily sprinkle descriptions where available in OpenAPI

70ec82f

Turn Descriptions to Maybe Texts

dc2a0a0

Get descriptions from pg_catalog.pg_description

9e2ae57

ldesgoui added 2 commits June 8, 2017 11:01

populate OpenAPI info.description with Schema description and paths./…

f27307f

…tablename.{get,post,put,delete}.description with Table description

Remove dead ToJSON instances

dd380de

ldesgoui changed the title ~~[WIP] Use Postgres' COMMENT feature to document OpenAPI~~ [WIP] Update OpenAPI (SQL COMMENT to description, constraints, cleaning up) Jun 8, 2017

fix hlint

17d992d

Reference table in response 200, move 200 to GET only

cf8989f

Fix tests by making deleteOp valid

b7404a9

ldesgoui added 2 commits July 17, 2017 05:18

External Docs to postgrest.com and describe root path

1e6effc

Remove duplication of parameters by using references

0fa5b83

ldesgoui closed this Jul 17, 2017

ldesgoui reopened this Jul 17, 2017

ldesgoui added 4 commits July 17, 2017 07:52

Merge branch 'master' into master

32735a6

hlint & move out of where when unneeded

bc293cf

Display if PK or FK in model descriptions

68a46d4

Merge branch 'master' of github.com:ldesgoui/postgrest

f1c56d7

ldesgoui added 4 commits July 19, 2017 16:35

Update tests to accomodate OpenAPI changes

3f8f472

changelog lineg

3042754

Revert "changelog lineg"

b95b9b8

This reverts commit 3042754.

Changelog line

5985e34

ldesgoui changed the title ~~[WIP] Update OpenAPI (SQL COMMENT to description, constraints, cleaning up)~~ Update OpenAPI (SQL COMMENT to description, constraints, cleaning up) Jul 19, 2017

hlint

98b57ad

begriffs reviewed Jul 19, 2017

View reviewed changes

simplify string conversion

0306bad

begriffs merged commit 6d5f72b into PostgREST:master Jul 22, 2017

begriffs mentioned this pull request Oct 8, 2017

Explain how sql comments appear in the openapi output PostgREST/postgrest-docs#111

Merged

This was referenced Apr 30, 2018

Feature request: Customizable OpenAPI title and description. #1100

Closed

OpenAPI: Document SQL comment on SCHEMA and TABLE comment splitting PostgREST/postgrest-docs#131

Closed

steve-chavez reviewed Aug 27, 2020

View reviewed changes

steve-chavez mentioned this pull request Sep 3, 2020

Options to integrate vendor extensions to generated OpenApi swagger documentation #1577

Open

steve-chavez mentioned this pull request Dec 20, 2020

"Note: this is a Primary Key" is not overridable with a comment #1700

Open

steve-chavez mentioned this pull request Jul 4, 2021

Correct openapi ignore-privs table comments #1886

Merged

monacoremo pushed a commit to monacoremo/postgrest that referenced this pull request Jul 17, 2021

Update OpenAPI (SQL COMMENT to description, constraints, cleaning up) (…

a816132

…PostgREST#885)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update OpenAPI (SQL COMMENT to description, constraints, cleaning up) #885

Update OpenAPI (SQL COMMENT to description, constraints, cleaning up) #885

ldesgoui commented Jun 3, 2017 •

edited

begriffs commented Jun 8, 2017

begriffs commented Jun 11, 2017

begriffs commented Jul 1, 2017

ldesgoui commented Jul 2, 2017

begriffs commented Jul 2, 2017

ldesgoui commented Jul 12, 2017 •

edited

ldesgoui commented Jul 17, 2017

ldesgoui commented Jul 17, 2017

begriffs commented Jul 17, 2017

ldesgoui commented Jul 18, 2017

ldesgoui commented Jul 19, 2017

begriffs Jul 19, 2017 •

edited

begriffs commented Jul 22, 2017

ldesgoui commented Jul 22, 2017

steve-chavez Aug 27, 2020

ldesgoui commented Aug 27, 2020 via email

steve-chavez commented Aug 27, 2020

Update OpenAPI (SQL COMMENT to description, constraints, cleaning up) #885

Update OpenAPI (SQL COMMENT to description, constraints, cleaning up) #885

Conversation

ldesgoui commented Jun 3, 2017 • edited

begriffs commented Jun 8, 2017

begriffs commented Jun 11, 2017

begriffs commented Jul 1, 2017

ldesgoui commented Jul 2, 2017

begriffs commented Jul 2, 2017

ldesgoui commented Jul 12, 2017 • edited

ldesgoui commented Jul 17, 2017

ldesgoui commented Jul 17, 2017

begriffs commented Jul 17, 2017

ldesgoui commented Jul 18, 2017

ldesgoui commented Jul 19, 2017

begriffs Jul 19, 2017 • edited

Choose a reason for hiding this comment

begriffs commented Jul 22, 2017

ldesgoui commented Jul 22, 2017

steve-chavez Aug 27, 2020

Choose a reason for hiding this comment

ldesgoui commented Aug 27, 2020 via email

steve-chavez commented Aug 27, 2020

ldesgoui commented Jun 3, 2017 •

edited

ldesgoui commented Jul 12, 2017 •

edited

begriffs Jul 19, 2017 •

edited