multiple schemas in one instance

[steve-chavez]

Continuing the work on #1436.

[steve-chavez]

@MahmoudKassem Regarding your issue on #1436 (comment)

implement a check that forbids cross schema queries to go through unless both source and target schema are explicitly added in the db-schema parameter

What I've done is to reject the request at the http level(it doesn't hit the database, so no need for tailoring an invalid sql query). I've taken into account the Accept-Profile proposal recommendations:

If the server cannot process the specified profile, it would answer with an http 406 status code and possibly a list of acceptable profiles.

So, if the user specifies an invalid schema he'll get an error:

curl -H "Accept-Version: v5" localhost:3000/table
{"message":"The schema must be one of the following: v1, v2"} 

I've also solved the dbstructure issue(mentioned in #1436 (comment)).

Let me know what you think.

[ghost]

@steve-chavez great job! Now all that's left is the OpenAPI test, right? Will try to get them done tomorrow.

[steve-chavez]

@MahmoudKassem I've noticed that with the Accept-Version approach we can GET different schemas resources. But how about a POST or PATCH to a different schema resource?
With the current interface that cannot be done.

(Edit: Actually, I've noted that it's possible to do this with the same Accept-Version header, but doing it this way wouldn't be complying to http semantics. Unless we add a Content-Version ourselves.)

However If we fully adopt the Content Negotiation by Profile spec we could have that working by doing:

POST /items
Content-Profile: v2 

{....}

That would make an insert to the v2.items relation.
I'll have to read the spec more carefully and see if we can do it that way.

For now, you adding the missing OpenAPI tests would be great.

[steve-chavez]

To fully adopt the profile negotiation spec, we would need to implement two requests: list profiles and get resource by profile.

I'm asking some questions to the workgroup(w3c/dx-connegp#15 and w3c/dx-connegp#18) for how to do this correctly.

As it is now, we can implement only the "get resource by profile" part(the Accept-Profile/Content-Profile requests, which would solve our issue).

I'd like to move forward and make a new release. We can work on supporting "list profiles" after the release and for now we'll make a note in the docs about it being a work in progress.

[ghost]

@steve-chavez the test is failing because it is omitting the "value" object in the properties as well as the "value" key in the "required" array, not sure what's wrong.

[ghost]

@steve-chavez in my local database(not the one created for the tests) the fields where marked as not null that's why they were showing up in the required field when calling the service with curl. Now that error is gone, but I don't understand why the values are omitted under "properties".

[steve-chavez]

I also don't have a clue why the value in properties might be missing. It's weird because when running postgrest executable I get the correct openapi output but on the test suite the value is missing.

[steve-chavez]

My only guess for now is that the DbStructure doesn't contain the columns(value) but it does contain the primary keys and somehow this is only happening when running the test suite. There must be something related to the config there. I'd have to traceShowId to see what's going on.

[steve-chavez]

Ahhh.. here's the culprit:

postgrest/test/Main.hs

Lines 58 to 60 in de218e9

	result <- P.use pool $ do
	ver <- getPgVersion
	HT.transaction HT.ReadCommitted HT.Read $ getDbStructure "test" ver

In the test suite the DbStructure is only loading the "test" schema. This needs refactoring so it doesn't happen again when testing other schemas.

[ghost]

@steve-chavez thank you for finding the issue, now that this is addressed its time to take a look at "get resource by profile" implementation.

[steve-chavez]

@MahmoudKassem Regarding "get resource by profile", for GET/HEAD should be simple. Basically:

GET /table
Accept-Profile: v1

HTTP/1.1 200 OK
Content-Profile: v1

body here

That is, when Accept-Profile is specified the response must contain a Content-Profile reply if it's possible to comply.

For POST/PATCH/PUT/DELETE, the implementation should be:

POST /table
Content-Profile: v1

body here

HTTP/1.1 201 Created

That should also be straightforward but there's a catch here.

Suppose we have a private.table, and we create a VIEW of this table in v1.table, another VIEW in v2.table and we expose these v1, v2 schemas through postgrest.

Should we allow to POST a payload to a v1.table and respond with a v2.table representation? In theory, this could be done with the following headers:

POST /table
Content-Profile: v1
Accept-Profile: v2
Prefer: return=representation

body here

For now I think we should not allow this and keep it simple. If a POST request with Content-Profile, Accept-Profile and Prefer: return=representation comes, assume the Content-Profile schema(ignore Accept-Profile) and add the Content-Profile: v1 header to the response as well.

[steve-chavez]

Just a note here, on how to implement the "list profiles" part.

First we would need a way to get the metadata for a single resource. Right now we offer metadata for all resources at the root. This was also requested in #1421.

I'm thinking that the link for getting a v1.table metadata should be:

GET /?resource=table&_profile=v1

(Borrowing the _profile query param from the spec)

This would give a filtered openapi output. Basically, the openapi paths, definitions and parameters objects would only contain info related to v1.table instead of containing the whole schema info.

Once we have that, "list profiles" would be done with OPTIONS:

OPTIONS /table

Accept: application/json, ...
Accept-Profile:
  /?resource=table&_profile=v1;
          rel="canonical";
          token="v1",
  /?resource=table&_profile=v2;
          rel="alternate";
          token="v2"

(I'm using relative URLs there, but they could be absolute like <http://example.org/?_profile=v2&resource=table>)

Once we have that I think we would be in compliance with the spec. It can be done in a later PR after #1421 is solved.

[steve-chavez]

Come to think of it, if we list profiles with OPTIONS that would also solve #790. Funny that #790 (comment) also mentioned the profile spec as a possible solution. Though it was an older one where the profile could only be specified in the media type.

[steve-chavez]

I've added a condition for checking profile negotiation is only possible when multiple schemas are specified. This is to ensure that users that would like to keep using a single schema don't see the Content-Profile: v1 response.

Tests that are missing: POST/PATCH/PUT/DELETE, calling a stored proc and maybe a resource embedding one to ensure it all works normally.

[steve-chavez]

I think I've finished my work here. The tests should cover all of the http verbs.

@MahmoudKassem Could you give it a final review and add an entry to the CHANGELOG?

[ghost]

@steve-chavez Everything looks fine to me, sorry for being so inactive things have been crazy lately and thank you for all your efforts, patience and all that I have learned. Glad to see this feature being seen through. I've added an entry to CHANGELOG.md , if you want to add something please feel free to do so.

[steve-chavez]

@MahmoudKassem Thanks a lot for your work and help here. Without your initiative this feature wouldn't have been done by now. So I'm also giving you credit on the CHANGELOG.

I'll merge and make a new release 🚀