Skip to content

Refactoring method addFieldIfNotExists #4461

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.

Sign up for GitHub

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

Merged
vitaly-t merged 4 commits into from
Dec 27, 2017
Merged

Refactoring method addFieldIfNotExists #4461

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40ad625
Refactoring method `addFieldIfNotExists`
vitaly-t Dec 26, 2017
9aa4f8d
Update PostgresStorageAdapter.js
vitaly-t Dec 26, 2017
95159d0
Merge branch 'master' into vitaly-t-patch-2
vitaly-t Dec 27, 2017
ed2a1b2
Update PostgresStorageAdapter.js
vitaly-t Dec 27, 2017
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
60 changes: 29 additions & 31 deletions src/Adapters/Storage/Postgres/PostgresStorageAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,8 +660,9 @@ export class PostgresStorageAdapter {
});
}

createClass(className, schema) {
return this._client.tx('create-class', t => {
createClass(className, schema, conn) {
conn = conn || this._client;
return conn.tx('create-class', t => {
const q1 = this.createTable(className, schema, t);
const q2 = t.none('INSERT INTO "_SCHEMA" ("className", "schema", "isParseClass") VALUES ($<className>, $<schema>, true)', { className, schema });
const q3 = this.setIndexesWithSchemaFormat(className, schema.indexes, {}, schema.fields, t);
Expand Down Expand Up @@ -746,40 +747,37 @@ export class PostgresStorageAdapter {
addFieldIfNotExists(className, fieldName, type) {
// TODO: Must be revised for invalid logic...
debug('addFieldIfNotExists', {className, fieldName, type});
return this._client.tx('add-field-if-not-exists', t => {
let promise = Promise.resolve();
const self = this;
return this._client.tx('add-field-if-not-exists', function * (t) {
if (type.type !== 'Relation') {
promise = t.none('ALTER TABLE $<className:name> ADD COLUMN $<fieldName:name> $<postgresType:raw>', {
className,
fieldName,
postgresType: parseTypeToPostgresType(type)
})
.catch(error => {
try {
yield t.none('ALTER TABLE $<className:name> ADD COLUMN $<fieldName:name> $<postgresType:raw>', {
className,
fieldName,
postgresType: parseTypeToPostgresType(type)
});
} catch(error) {
if (error.code === PostgresRelationDoesNotExistError) {
return this.createClass(className, {fields: {[fieldName]: type}})
} else if (error.code === PostgresDuplicateColumnError) {
// Column already exists, created by other request. Carry on to
// See if it's the right type.
} else {
throw error;
return yield self.createClass(className, {fields: {[fieldName]: type}}, t);
}
})
if (error.code === PostgresDuplicateColumnError) {
// Column already exists, created by other request. Carry on to
// See if it's the right type.
Copy link
Copy Markdown
Contributor

@flovilmart flovilmart Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return here and not propagate the error to match original implementation. Otherwise, remove altogether.

Copy link
Copy Markdown
Contributor Author

@vitaly-t vitaly-t Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I did match the original implementation. Was it somehow wrong?

Copy link
Copy Markdown
Contributor

@flovilmart flovilmart Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original implementation, the throw is in an else statement, so never executed if it enters the DuplicateColumnError check

}
throw error;
};
} else {
promise = t.none('CREATE TABLE IF NOT EXISTS $<joinTable:name> ("relatedId" varChar(120), "owningId" varChar(120), PRIMARY KEY("relatedId", "owningId") )', {joinTable: `_Join:${fieldName}:${className}`})
yield t.none('CREATE TABLE IF NOT EXISTS $<joinTable:name> ("relatedId" varChar(120), "owningId" varChar(120), PRIMARY KEY("relatedId", "owningId") )', {joinTable: `_Join:${fieldName}:${className}`});
}

const result = yield t.any('SELECT "schema" FROM "_SCHEMA" WHERE "className" = $<className> and ("schema"::json->\'fields\'->$<fieldName>) is not null', {className, fieldName});

if (result[0]) {
throw 'Attempted to add a field that already exists';
} else {
const path = `{fields,${fieldName}}`;
yield t.none('UPDATE "_SCHEMA" SET "schema"=jsonb_set("schema", $<path>, $<type>) WHERE "className"=$<className>', {path, type, className});
}
return promise.then(() => {
return t.any('SELECT "schema" FROM "_SCHEMA" WHERE "className" = $<className> and ("schema"::json->\'fields\'->$<fieldName>) is not null', {className, fieldName});
}).then(result => {
if (result[0]) {
throw "Attempted to add a field that already exists";
} else {
const path = `{fields,${fieldName}}`;
return t.none(
'UPDATE "_SCHEMA" SET "schema"=jsonb_set("schema", $<path>, $<type>) WHERE "className"=$<className>',
{ path, type, className }
);
}
});
});
}

Expand Down