Add option to retain patch/update identifier safety in objection 0.7.0 #2

Merged
merged 1 commit into from Aug 10, 2017

Conversation

Projects
None yet
2 participants
@snlamm
Member

snlamm commented Apr 4, 2017

Addresses: This PR assumes that at some point grind-orm will upgrade to objection 0.7.*.
One of the breaking changes with 0.7.0 is that patch/update operations can now change identifier columns. In other words, the id column can be changed accidentally. Previously, if an id column was in a patch/update operation it was ignored.

This safety feature seems useful as it's generally considered bad practice to change a model's identifier, so cases when this happens will often be unintentional and destructive.

This PR adds a non-default option to preserve the patch/update safety feature. At the same time, it remains compatible with grind-orm using objection 0.6.* (i.e. it doesn't add extra overhead).

Changes: adds a new static variable to Model called useIdSafeUpdates, which defaults to false. When set to true, ID columns will be stripped from patch/update queries using objection's previously included logic.

Ex.

import { Model } from 'grind-orm'

Model.useIdSafeUpdates = true

Since the default for useIdSafeUpdates is false, it is compatible with 0.6.*, where identifier safety is already default behavior so setting useIdSafeUpdates is unnecessary. Once using objection 0.7.0, setting it to true will preserve the previous safety behavior.

src/Model.js
+ if(this.constructor.useIdSafeUpdates) {
+ const cols = this.constructor.getIdColumnArray()
+
+ for(let i = 0, l = cols.length; i < l; i++) {

This comment has been minimized.

@shnhrrsn

shnhrrsn Apr 16, 2017

Member

I think we should include a fast path here when there’s only a single id column, given that most cases with overwhelmingly be just id. We can fall back to the for loop in cases where it’s more than one.

@shnhrrsn

shnhrrsn Apr 16, 2017

Member

I think we should include a fast path here when there’s only a single id column, given that most cases with overwhelmingly be just id. We can fall back to the for loop in cases where it’s more than one.

This comment has been minimized.

@snlamm

snlamm Apr 28, 2017

Member

thanks. updated the commit

@snlamm

snlamm Apr 28, 2017

Member

thanks. updated the commit

src/Model.js
+ if(this.constructor.useIdSafeUpdates) {
+ const cols = this.constructor.getIdColumnArray()
+
+ if((cols.length === 1) && !this.id.isNil) {

This comment has been minimized.

@shnhrrsn

shnhrrsn May 5, 2017

Member

I think this should be:

if(cols.length === 1) {
	delete this[cols[0]]
} else {
	for(let i = 0, l = cols.length; i < l; i++) {
		delete this[cols[i]]
	}
}
@shnhrrsn

shnhrrsn May 5, 2017

Member

I think this should be:

if(cols.length === 1) {
	delete this[cols[0]]
} else {
	for(let i = 0, l = cols.length; i < l; i++) {
		delete this[cols[i]]
	}
}

This comment has been minimized.

@snlamm

snlamm May 10, 2017

Member

made the changes

@snlamm

snlamm May 10, 2017

Member

made the changes

slamm
add option to prevent model identifier from being changed through pat…
…ch/update operation. Preserves safety feature removed from objection 0.7.0. includes fastrack for 'id' column

@shnhrrsn shnhrrsn merged commit 9b12f0c into grindjs:master Aug 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment