Auth.verbsOn(): deduplicate data returned from db#1503
Auth.verbsOn(): deduplicate data returned from db#1503alxndrsn merged 2 commits intogetodk:masterfrom
Conversation
Resolves TODO ref de-duplicating data on postgres side. Noticed while working on getodk#1502
| project.verbs.should.be.an.Array(); | ||
| const { body: admin } = await asAlice.get('/v1/roles/admin').expect(200); | ||
| project.verbs.should.eql(admin.verbs); | ||
| project.verbs.should.eqlInAnyOrder(admin.verbs); |
There was a problem hiding this comment.
Probably doesn't matter, but do you know why this changed? Why is order relevant now when it wasn't before? It only looks like there's one role involved (Alice is just an admin, I believe).
Maybe it's because SELECT DISTINCT deduplicates differently from Ramda uniq? Like maybe SELECT DISTINCT takes the first duplicate, while Ramda takes the last?
One thing that I think is a little important is that the API response is consistent. Like if nothing changes, I would hope that /v1/projects/1 would return the same verbs in the same order for each request. It's OK if that order is different from before, but consistency from one request to the next seems nice to me. Maybe even that isn't essential, but it'd be good to know if that's changed.
There was a problem hiding this comment.
I don't think SQL gives any ordering guarantees unless an ORDER BY clause is supplied.
E.g. from https://www.postgresql.org/docs/current/queries-order.html:
If sorting is not chosen, the rows will be returned in an unspecified order.
Like if nothing changes, I would hope that /v1/projects/1 would return the same verbs in the same order for each request.
This is likely to be true in general, although it's always possible for something to change internally in postgres:
The actual order in that case will depend on the scan and join plan types and the order on disk, but it must not be relied on. A particular output ordering can only be guaranteed if the sort step is explicitly chosen.
(Again from https://www.postgresql.org/docs/current/queries-order.html.)
There was a problem hiding this comment.
I don't think SQL gives any ordering guarantees unless an
ORDER BYclause is supplied.
Maybe that's a slight downside of the new approach. Now that the deduplicating is done in Postgres, order is not guaranteed. Whereas before, the order was consistent in many cases, e.g., if the user was only assigned a single role. Previously, Postgres returned verbs as an array without changing the internal order of the array.
I still feel like it's not a big deal though. If it becomes an issue or source of confusion, we can always add ordering guarantees later.
There was a problem hiding this comment.
Maybe that's a slight downside of the new approach.
Did the old approach provide any guarantees about ordering?
Resolves TODO ref de-duplicating data on postgres side.
Noticed while working on #1502
What has been done to verify that this works as intended?
All existing tests continue to pass.
Why is this the best possible solution? Were any other approaches considered?
The functional change is quite minimal. There are unnecessary formatting changes included in this PR, which IMO make the whole thing more readable.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
No effect.
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
No.
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass OR confirm CircleCI build passes