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

add new test cases for location header #1475

Merged
merged 11 commits into from
Jul 8, 2020

Conversation

wolfgangwalther
Copy link
Member

Just some test cases as a head start on #1461 for whoever wants to work on it (might be me, might be somebody else).

see also #1471 (comment)

@wolfgangwalther
Copy link
Member Author

@steve-chavez can you have a look at the failed style-check. It suggests "to reduce duplication", just because two different test-cases expect the same results and have 3 rows in common, I guess.

How do I handle that?

@wolfgangwalther
Copy link
Member Author

So I added/fixed some more test cases and implemented a part of it: location headers are currently only created for primary key columns. Not for any other columns anymore, but also not for unique columns, in case a pk is not available. That would still be to do, but requires more work, I guess, because the list of unique columns first needs to be made available there.

Also fixed #1162, by always adding the primary key cols to the RETURNING part. This will have to be extended with unique columns, once those are used for location headers as well. I wonder whether we should maybe just do RETURNING * all the time, that would simplify that a lot and those unneeded columns will still not be selected with return=minimal or select=x,y,z, because the outer query does a SELECT x, y, z FROM .. anyways... - what do you think @steve-chavez?

Still have two tests failing right now. One is expected (unique column fallback), the other one is not (unicode). @steve-chavez could you have a look at that 2nd one? Basically the response is correct, the json comparison is just in wrong order. No matter in which order I put the keys in the let payload = ..., they always come out the same way (wrong!). I also tried creating the response string to match by hand, instead of reusing payload, but that somehow generated different escape sequences for those unicode characters, so it failed as well. How do I do that properly?

@steve-chavez
Copy link
Member

@wolfgangwalther Regarding unique columns, I also think it'd be better to leave that addition for later.

I wonder whether we should maybe just do RETURNING * all the time, that would simplify that a lot and those unneeded columns will still not be selected with return=minimal or select=x,y,z, because the outer query does a SELECT x, y, z FROM .. anyways

Not sure if RETURNING * would pass all the tests, some could fail because of column privileges. But if you could make it work, by all means go ahead and I'll give it a review.

@wolfgangwalther
Copy link
Member Author

Column privileges! Yes, that's a very good reason for what we have right now. I don't think those would work with RETURNING *, so I won't even try it.

I removed the tests for "location headers from unique columns" for now, so this should be good to merge.

Resolves #1461, #1162

@steve-chavez
Copy link
Member

@wolfgangwalther Made one last review, check my above comments.

Also, please add the fixes to the CHANGELOG and then I'll merge!

@wolfgangwalther
Copy link
Member Author

wolfgangwalther commented Jul 7, 2020

Technically one of them is a change not a fix, I guess, because if you were to make a really large TEXT column the PK on the table the actual bug in #1461 (exceeding header size limit) would still not be solved. So the "bug" is still there, although a lot less likely with the changed behaviour ;)

@steve-chavez steve-chavez merged commit 343e41c into PostgREST:master Jul 8, 2020
@steve-chavez
Copy link
Member

@wolfgangwalther Thanks!

because if you were to make a really large TEXT column the PK on the table

I think it's unlikely to have such a big text PK. So I'm calling #1461 fixed :)

steve-chavez pushed a commit that referenced this pull request Jun 18, 2021
monacoremo pushed a commit to monacoremo/postgrest that referenced this pull request Jul 17, 2021
* Create location header only on primary key columns, closes PostgREST#1461

* Return location header when pk columns not selected, fixes PostgREST#1162
monacoremo pushed a commit to monacoremo/postgrest that referenced this pull request Jul 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants