-
-
Notifications
You must be signed in to change notification settings - Fork 568
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
Only include columns in insert statements if needed #367
Conversation
Columns that have no specific value set (are undefined) previously were excplicitly set to `default`. However, this causes problems if you do not have permission to set this column, whereas not specifing the column at all works fine (and still ends up with the default value). This changes that, by only including columns in the insert statement if they have a value set. If the insert statement is for multiple rows, and not all rows have the same columns, all columns that are present in at least one row will be included. Rows that don't have that column specified will revert to the old behavior, being set to `default` explicitly.
This looks good to me 👍 Could you add a test that fails on master and passes on this branch for this code please, to avoid regressions? |
Sure, I'll get on that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good 👍
I’m glad you submit this PR, because I was actually thinking about this in a discussion I was having 😊. This is required for our support for column GRANT
s in inserts, right?
Just one comment. I’m not super excited about the time complexity of iterating over all the values and then all of the values fields, however I’m fine with merging it for now with the intention of optimizing when it becomes a problem.
Anyway, I’ll be happy to merge this.
(fun fact: as I was writing this review I saw you added tests 😉)
map.forEach((value, key) => { | ||
if (typeof value !== 'undefined') { | ||
fields.set(key, this.type.fields.get(key)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My one concern is that this is O(vn)
where v
is the number of values and n
is the maximum number of fields in one of the values. Is there a more efficient algorithm for this? Can we do some other work while iterating? Think about these questions only for a bit, if you can’t think of a good answer I’m comfortable with merging this as an initial implementation.
Ideally I think we could take a similar approach here as we are taking in #342, although this is a good first implementation. If you could put a TODO
comment here that reads something like: TODO: Use field information from GraphQL instead of iterating through values
we can address it when it becomes a problem 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't really considered that yet, but I can see how that could become an issue. I unfortunately do not see a way to avoid that with this approach.
Another approach to this problem would be to detect which columns can be inserted on through introspection instead. This only has to be done at startup/watch updates, and does not involve extra looping, so this would effectively be O(1)
. We would go back to sometimes specifying default
for columns that we could leave out completely, but as long as you have the rights to set those columns this is fine. I am not (yet) familiar enough with the project to achieve this though, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another benefit to detecting these columns through introspection would be that this could also be reflected in the GraphQL schema/docs. We could simply leave columns that cannot be set out of the Input
objects, thereby also preventing this from being a runtime issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work for my schema; we use multiple roles and those roles have different column access grants. Unless you enumerated all of the roles with access, and applied those learnings at run-time; but that seems complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I failed to consider using multiple roles. That will indeed complicate things significantly, to the point where it may not be worth it at all. I think @calebmer's proposal to, if the performance of this code ever becomes an issue, use the field information from GraphQL is probably best then. For now I'll just add a TODO
comment about it.
collection1.create(context, value3), | ||
]) | ||
|
||
const query = client.query.mock.calls[0][0].text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you either check the entire text of the query (vs. using toMatch
) with either an inline string literal, or a snapshot? expect(client.query.mock.calls[0][0].text).toMatchSnapshot()
. Snapshot is probably the easiest of course. In the future I’d like to be snapshotting more of the SQL queries we generate.
Could you also add a sanity check here: expect(client.query.mock.calls.length).toBe(1)
just so that we can be sure that no other queries are being run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had that in there initially for that exact reason, but I seem to have removed it accidentally. Whoops! Will add it back.
This is indeed needed for |
Also added a sanity check to make sure exactly one (1) query has been executed
Awesome, this resolves an issue I've had on another project also (again: updatedAt and createdAt). Thanks! 👍 |
Released in 3.1.0 🎉 |
…raphile#367) * Only include required columns in insert statments Columns that have no specific value set (are undefined) previously were excplicitly set to `default`. However, this causes problems if you do not have permission to set this column, whereas not specifing the column at all works fine (and still ends up with the default value). This changes that, by only including columns in the insert statement if they have a value set. If the insert statement is for multiple rows, and not all rows have the same columns, all columns that are present in at least one row will be included. Rows that don't have that column specified will revert to the old behavior, being set to `default` explicitly. * Added a test case * Use snapshots for the test Also added a sanity check to make sure exactly one (1) query has been executed * Added a TODO comment about optimisation
Columns that have no specific value set (are
undefined
) previously were explicitly set todefault
. However, this causes problems if you do not have permission to set a column, whereas if these columns are not specified at all it works fine, and they will still ends up with their default value.This changes that, by only including columns in the insert statement if they have a value set. If the insert statement is for multiple rows, and not all rows have the same columns, all columns that are present in at
least one row will be included. Rows that don't have that column specified will revert to the old behaviour, being set to
default
explicitly.An example: say you have a table
foo
with the columnsname
andsecret
. You're allowed to read all of them, but you're only allowed to setname
. Previously, if you call the mutationcreateFoo
with only a value forname
, the query will look something like:This will cause an error, as you're not allowed to set the column
secret
. After this change, the query will look something like this:This will work fine.