fix: fix OpenAPI pk/fk note not being overridable

[steve-chavez]

Resolves #1700.

I've just allowed overriding the notes with the db comment.

I looked into adding x-primary-key or x-foreign-key fields to not lose the metadata, but seems this is not easy with our current swagger lib: GetShopTV/swagger2#113 (comment).

Still I think this is a better behavior, I'll leave adding the extensions for our future OpenAPI-in-SQL #1698.

[wolfgangwalther]

Uh. This immediately breaks my client lib vue-postgrest, once a comment on a PK column is added. I rely on <pk/> to be there, to identify those columns.

[steve-chavez]

@wolfgangwalther Ah, I didn't think anyone was relying on those notes. Would adding an x-primary-key: true work for you?

Also, do you use the foreign key <fk/> note?

(I see it's used here: https://github.com/technowledgy/vue-postgrest/blob/4dd955799dddcdffafc022264eb1242bddb2e2eb/src/Route.js#L22)

[wolfgangwalther]

@wolfgangwalther Ah, I didn't think anyone was relying on those notes. Would adding an x-primary-key: true work for you?

I haven't looked deeper into that, but I guess it would. I just need to know which columns identify records on a route uniquely, so that I can use those for all my model.$patch(..), model.$delete(...), model.$get(...), etc. calls. I am using the nightly in a project currently under development, so it would be cool if we had an immediate replacement when removing this.

Also, do you use the foreign key <fk/> note?

Nope, I can't imagine a use-case. I would hope to have "embedding opportunities" available in the future, that would help. Better to know about those before, than just after a 300 response... But I'm not going to derive those from <fk/> ;).

[steve-chavez]

Since vendor extensions are not supported GetShopTV/swagger2#4 (and there's no workaround but to patch upstream). I'm thinking about adding the pk/fk notes on the additionalProperties field, like:

curl -s localhost:3000 | jq .definitions.articles.properties
{
  "id": {
    "format": "integer",
    "additionalProperties": {
      "description": "Note:\nThis is a Primary Key.<pk/>\n"
    },
    "type": "integer",
    "description": "An article primary key"
  },
  "body": {
    "format": "text",
    "type": "string"
  },
  "owner": {
    "format": "name",
    "type": "string"
  }
}

@wolfgangwalther Would that work for you?

[steve-chavez]

I think it'd just be a matter of changing:

//from
fieldDef.description?.includes('<pk/>'))

//to 
fieldDef.additionalProperties.description?.includes('<pk/>'))

On https://github.com/technowledgy/vue-postgrest/blob/4dd955799dddcdffafc022264eb1242bddb2e2eb/src/Route.js#L22.

So I'll go ahead with the change :)

[wolfgangwalther]

Since vendor extensions are not supported GetShopTV/swagger2#4 (and there's no workaround but to patch upstream). I'm thinking about adding the pk/fk notes on the additionalProperties field, like:

curl -s localhost:3000 | jq .definitions.articles.properties
{
  "id": {
    "format": "integer",
    "additionalProperties": {
      "description": "Note:\nThis is a Primary Key.<pk/>\n"
    },
    "type": "integer",
    "description": "An article primary key"
  },
  "body": {
    "format": "text",
    "type": "string"
  },
  "owner": {
    "format": "name",
    "type": "string"
  }
}

@wolfgangwalther Would that work for you?

I couldn't find any docs on additionalProperties in that context yet... but if this field does in fact allow arbitrary keys, like the name suggests, why don't you do it like this?

"id": {
    "format": "integer",
    "additionalProperties": {
      "pk": true
    },
    "type": "integer",
    "description": "An article primary key"
  },

[steve-chavez]

Not possible to add a pk field like that, the types are too strict: https://hackage.haskell.org/package/swagger2-2.6/docs/Data-Swagger.html#t:Schema

Also, the description format doesn't change in this PR. That would help other users that also relied on that behavior, easier migration.

Besides that, I don't think it's worth to put much work on this since #1698 is the way forward.

[wolfgangwalther]

But to be honest, I don't think additionalProperties is to be used in either of those two ways. This is something entirely different, and not valid for a "type": "integer" at all.

My understanding is that additionalProperties allows to define a schema for additional properties of a field with "type": "object". Using a description on this non-existent schema to somehow hide the <pk/> information.... sounds really fishy.

What is the urgency in removing this from the description? Does the "Note: ..." break any usecase? Why can't it wait until we solve the openapi stuff properly in SQL and we don't have to add some strange hacks here?

[wolfgangwalther]

Not possible to add a pk field like that, the types are too strict: https://hackage.haskell.org/package/swagger2-2.6/docs/Data-Swagger.html#t:Schema

Ah yes, those Types match my expectation and just confirm that adding the note in the description of an otherwise non-existent sub-schema is really... not valid openapi at all.

[wolfgangwalther]

To summarize: It feels like this PR is just replacing one hack with another, worse, hack. It was never the best idea to add <pk/> in the description in the first place, but it is certainly a worse idea to add it in additionalProperties.description.

[wolfgangwalther]

I only need <pk/> though, so maybe it would be enough to just remove the whole "Note: This is a Primary Key." thing and only add the <pk/>? It would certainly be less obtrusive.

[steve-chavez]

To summarize: It feels like this PR is just replacing one hack with another, worse, hack.

Yeah, my original intention was to remove the hack. But since that breaks vue-postgrest I went with the additionalProperties workaround. As you mention that seems invalid for an integer type, so I'll not proceed because maybe it could break other stuff as well(like code generators).

I'll just close this one. Let's solve this later as part of #1698.

[steve-chavez]

Does the "Note: ..." break any usecase?

Ah, yes. It breaks OpenAPI UIs, as the notes don't go away despite adding a comment for the column. The <pk>/<fk/> look a bit like "garbage" for the end user.

This happens for supabase.io, which shows our OpenAPI comments.

Not sure how users of Swagger UI are cleaning the comments or if it was ever an issue there. But in Supabase's case, the comments are now cleaned with js on frontend.

[robertsosinski]

Adding on here, would it be possible to override this behavior with something like: comment on column <insert column here>.id is 'Unique id for this table.';? I can see the current Note + as a sensible default (as well as for backwards compatibility). But if I want to change this behavior, would be great to be able to, and if I want the "" there I can add it in my own comment.

Thanks!