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

Only include columns in insert statements if needed #367

Merged
merged 6 commits into from
Mar 5, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions src/postgres/inventory/collection/PgCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import DataLoader = require('dataloader')
import { Client } from 'pg'
import { Collection } from '../../../interface'
import { sql, memoizeMethod } from '../../utils'
import { PgCatalog, PgCatalogNamespace, PgCatalogClass, PgCatalogAttribute } from '../../introspection'
import { PgCatalog, PgCatalogNamespace, PgCatalogClass } from '../../introspection'
import PgClassType from '../type/PgClassType'
import Options from '../Options'
import pgClientFromContext from '../pgClientFromContext'
Expand All @@ -27,7 +27,6 @@ class PgCollection implements Collection<PgClassType.Value> {
* of `PgCatalog`.
*/
private _pgNamespace: PgCatalogNamespace = this._pgCatalog.assertGetNamespace(this.pgClass.namespaceId)
private _pgAttributes: Array<PgCatalogAttribute> = this._pgCatalog.getClassAttributes(this.pgClass.id)

/**
* The name of this collection. A pluralized version of the class name. We
Expand Down Expand Up @@ -133,23 +132,36 @@ class PgCollection implements Collection<PgClassType.Value> {
async (values: Array<PgClassType.Value>): Promise<Array<PgClassType.Value>> => {
const insertionIdentifier = Symbol()

// Get the fields that actually have been used in any of the values, so
// we can leave the rest out of the query completely to prevent
// permission issues
// TODO: Use field information from GraphQL instead of iterating through values
const fields = new Map()
values.forEach((map) => {
map.forEach((value, key) => {
if (typeof value !== 'undefined') {
fields.set(key, this.type.fields.get(key))
}
Copy link
Collaborator

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 👍

Copy link
Author

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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.

})
})

// Create our insert query.
const query = sql.compile(sql.query`
with ${sql.identifier(insertionIdentifier)} as (
-- Start by defining our header which will be the class we are
-- inserting into (prefixed by namespace of course).
insert into ${sql.identifier(this._pgNamespace.name, this.pgClass.name)}

-- Add all of our attributes as columns to be inserted into. This is
-- helpful in case the columns differ from what we expect.
(${sql.join(this._pgAttributes.map(({ name }) => sql.identifier(name)), ', ')})
-- Add all of the attributes that we're going to use as columns to be inserted into.
-- This is helpful in case the columns differ from what we expect.
(${sql.join(Array.from(fields.values()).map(({ pgAttribute }) => sql.identifier(pgAttribute.name)), ', ')})

-- Next, add all of our value tuples.
values ${sql.join(values.map(value =>
// Make sure we have one value for every attribute in the class,
// if there was no such value defined, we should just use
// `default` and use the user’s default value.
sql.query`(${sql.join(Array.from(this.type.fields.values()).map(field => {
sql.query`(${sql.join(Array.from(fields.values()).map(field => {
const fieldValue = field.getValue(value)
return typeof fieldValue === 'undefined' ? sql.query`default` : field.type.transformValueIntoPgValue(fieldValue)
}), ', ')})`,
Expand Down
20 changes: 20 additions & 0 deletions src/postgres/inventory/collection/__tests__/PgCollection-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,26 @@ test('create will insert new rows into the database', withPgClient(async client
.toEqual(values.map(mapToObject))
}))

test('create will only include relevant columns', withPgClient(async client => {
const context = { [$$pgClient]: client }

// Note how the about column is not used
const value1 = new Map([['name', 'John Smith'], ['email', 'john.smith@email.com']])
const value2 = new Map([['name', 'Sarah Smith'], ['email', 'sarah.smith@email.com']])
const value3 = new Map([['name', 'Budd Deey'], ['email', 'budd.deey@email.com']])

client.query.mockClear()

await Promise.all([
collection1.create(context, value1),
collection1.create(context, value2),
collection1.create(context, value3),
])

expect(client.query.mock.calls.length).toBe(1)
expect(client.query.mock.calls[0][0].text).toMatchSnapshot()
}))

// TODO: reimplement
// test('paginator will have the same name and type', () => {
// expect(collection1.paginator.name).toBe(collection1.name)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exports[`test create will only include relevant columns 1`] = `"with __local_0__ as ( insert into \"c\".\"person\" (\"name\", \"email\") values ($1, $2), ($3, $4), ($5, $6) returning * ) select row_to_json(__local_0__) as object from __local_0__"`;