Skip to content

Commit

Permalink
fixes Vincit#1455
Browse files Browse the repository at this point in the history
  • Loading branch information
koskimas committed Sep 21, 2019
1 parent 52eebdc commit f328630
Show file tree
Hide file tree
Showing 10 changed files with 217 additions and 33 deletions.
4 changes: 4 additions & 0 deletions doc/changelog/README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## 1.6.10

* Fixes [#1455](https://github.com/Vincit/objection.js/issues/1455)

## 1.6.9

* Revert fix for [#1089](https://github.com/Vincit/objection.js/issues/1089). It was causing more bugs than it fixed. #1089 will be addressed in 2.0.
Expand Down
4 changes: 1 addition & 3 deletions lib/queryBuilder/QueryBuilder.js
Original file line number Diff line number Diff line change
Expand Up @@ -1190,9 +1190,7 @@ function parseRelationExpression(modelClass, exp) {
if (err instanceof DuplicateRelationError) {
throw modelClass.createValidationError({
type: ValidationErrorType.RelationExpression,
message: `Duplicate relation name "${
err.relationName
}" in relation expression "${exp}". Use "a.[b, c]" instead of "[a.b, a.c]".`
message: `Duplicate relation name "${err.relationName}" in relation expression "${exp}". Use "a.[b, c]" instead of "[a.b, a.c]".`
});
} else {
throw modelClass.createValidationError({
Expand Down
26 changes: 25 additions & 1 deletion lib/queryBuilder/graph/GraphUpsert.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,28 @@ function pruneRelatedBranches(graph, currentGraph, graphOptions) {

function pruneDeletedBranches(graph, currentGraph) {
const deleteNodes = currentGraph.nodes.filter(currentNode => !graph.nodeForNode(currentNode));
const roots = findRoots(deleteNodes);

// Don't delete relations the current graph doesn't even mention.
// So if the parent node doesn't even have the relation, it's not
// supposed to be deleted.
const rootsNotInRelation = roots.filter(deleteRoot => {
if (!deleteRoot.parentNode) {
return false;
}

const { relation } = deleteRoot.parentEdge;
const parentNode = graph.nodeForNode(deleteRoot.parentNode);

if (!parentNode) {
return false;
}

removeBranchesFromGraph(findRoots(deleteNodes), currentGraph);
return parentNode.obj[relation.name] === undefined;
});

removeBranchesFromGraph(roots, currentGraph);
removeNodesFromGraph(new Set(rootsNotInRelation), currentGraph);
}

function findRoots(nodes) {
Expand Down Expand Up @@ -125,6 +145,10 @@ function removeBranchesFromGraph(branchRoots, graph) {
)
);

removeNodesFromGraph(nodesToRemove, graph);
}

function removeNodesFromGraph(nodesToRemove, graph) {
const edgesToRemove = new Set();

for (const node of nodesToRemove) {
Expand Down
4 changes: 1 addition & 3 deletions lib/queryBuilder/parsers/relationExpressionParser.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 2 additions & 6 deletions lib/relations/Relation.js
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,7 @@ function createJoinProperties(ctx) {

if (ownerProp.props.some(it => it === ctx.name)) {
throw ctx.createError(
`join: relation name and join property '${
ctx.name
}' cannot have the same name. If you cannot change one or the other, you can use $parseDatabaseJson and $formatDatabaseJson methods to convert the column name.`
`join: relation name and join property '${ctx.name}' cannot have the same name. If you cannot change one or the other, you can use $parseDatabaseJson and $formatDatabaseJson methods to convert the column name.`
);
}

Expand All @@ -358,9 +356,7 @@ function createRelationProperty(ctx, refString, propName) {
} catch (err) {
if (err instanceof RelationProperty.ModelNotFoundError) {
throw ctx.createError(
`join: either \`from\` or \`to\` must point to the owner model table and the other one to the related table. It might be that specified table '${
err.tableName
}' is not correct`
`join: either \`from\` or \`to\` must point to the owner model table and the other one to the related table. It might be that specified table '${err.tableName}' is not correct`
);
} else if (err instanceof RelationProperty.InvalidReferenceError) {
throw ctx.createError(
Expand Down
4 changes: 1 addition & 3 deletions lib/utils/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ function assertHasId(model) {
const ids = modelClass.getIdColumnArray().join(', ');

throw new Error(
`one of the identifier columns [${ids}] is null or undefined. Have you specified the correct identifier column for the model '${
modelClass.name
}' using the 'idColumn' property?`
`one of the identifier columns [${ids}] is null or undefined. Have you specified the correct identifier column for the model '${modelClass.name}' using the 'idColumn' property?`
);
}
}
Expand Down
18 changes: 9 additions & 9 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "objection",
"version": "1.6.9",
"version": "1.6.10",
"description": "An SQL-friendly ORM for Node.js",
"main": "lib/objection.js",
"license": "MIT",
Expand Down Expand Up @@ -70,26 +70,26 @@
},
"devDependencies": {
"@babel/polyfill": "^7.4.4",
"@types/node": "^10.14.7",
"@types/node": "^12.7.5",
"babel-eslint": "^10.0.1",
"chai": "^4.2.0",
"chai-subset": "^1.6.0",
"coveralls": "^3.0.3",
"cross-env": "^5.2.0",
"eslint": "^5.16.0",
"cross-env": "^6.0.0",
"eslint": "^6.4.0",
"eslint-plugin-prettier": "^3.1.0",
"expect.js": "^0.3.1",
"fs-extra": "^7.0.1",
"fs-extra": "^8.1.0",
"glob": "^7.1.3",
"knex": "0.17.0",
"mocha": "^5.2.0",
"knex": "0.19.4",
"mocha": "^6.2.0",
"mysql": "^2.17.1",
"nyc": "^14.1.1",
"pg": "^7.11.0",
"prettier": "1.17.1",
"prettier": "1.18.2",
"sqlite3": "^4.0.8",
"typescript": "^3.4.5",
"vuepress": "0.14.11"
"vuepress": "1.1.0"
},
"nyc": {
"description": "test coverage",
Expand Down
166 changes: 166 additions & 0 deletions tests/integration/misc/#1455.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
const { Model, transaction } = require('../../../');
const { expect } = require('chai');

module.exports = session => {
describe('UpsertGraph deletes rows for relation which is not mentioned in graph #1455', () => {
let knex = session.knex;
let Role;

beforeEach(() => {
const { knex } = session;

return knex.schema
.dropTableIfExists('roles')
.then(() => knex.schema.dropTableIfExists('sets'))
.then(() => knex.schema.dropTableIfExists('setsAttributes'))
.then(() => {
return knex.schema.createTable('roles', table => {
table.increments();
table.string('name').notNullable();
});
})
.then(() => {
return knex.schema.createTable('sets', table => {
table.increments();
table.string('name').notNullable();
table
.integer('roleId')
.unsigned()
.notNullable();
});
})
.then(() => {
return knex.schema.createTable('setsAttributes', table => {
table.increments();
table.string('name').notNullable();
table
.integer('setId')
.unsigned()
.notNullable();
});
});
});

afterEach(() => {
return knex.schema
.dropTableIfExists('roles')
.then(() => knex.schema.dropTableIfExists('sets'))
.then(() => knex.schema.dropTableIfExists('setsAttributes'));
});

beforeEach(() => {
const { knex } = session;

class BaseModel extends Model {
static get useLimitInFirst() {
return true;
}
}

class SetAttribute extends BaseModel {
static get tableName() {
return 'setsAttributes';
}
}

class Set extends BaseModel {
static get tableName() {
return 'sets';
}

static get relationMappings() {
return {
setAttributes: {
relation: BaseModel.HasManyRelation,
modelClass: SetAttribute,
join: { from: 'sets.id', to: 'setsAttributes.setId' }
}
};
}
}

Role = class Role extends BaseModel {
static get tableName() {
return 'roles';
}

static get relationMappings() {
return {
sets: {
relation: BaseModel.HasManyRelation,
modelClass: Set,
join: { from: 'roles.id', to: 'sets.roleId' }
}
};
}
};

BaseModel.knex(knex);
});

it('test', () => {
return transaction(Role.knex(), trx =>
Role.query(trx).insertGraph({
name: 'First Role',
sets: [
{
name: 'First Set',
setAttributes: [{ name: 'First SetAttribute' }, { name: 'Second SetAttribute' }]
}
]
})
)
.then(role => {
return transaction(Role.knex(), trx =>
Role.query(trx).upsertGraph({
id: role.id,
sets: [
{ id: role.sets[0].id },
{
name: 'Second Set',
setAttributes: [{ name: 'First SetAttribute' }, { name: 'Second SetAttribute' }]
}
]
})
);
})
.then(() => {
return Role.query()
.first()
.eager('sets(orderById).setAttributes(orderById)', {
orderById(query) {
query.orderBy('id');
}
});
})
.then(setsAfterUpsertGraph => {
expect(setsAfterUpsertGraph).to.eql({
id: 1,
name: 'First Role',
sets: [
{
id: 1,
name: 'First Set',
roleId: 1,

setAttributes: [
{ id: 1, name: 'First SetAttribute', setId: 1 },
{ id: 2, name: 'Second SetAttribute', setId: 1 }
]
},
{
id: 2,
name: 'Second Set',
roleId: 1,

setAttributes: [
{ id: 3, name: 'First SetAttribute', setId: 2 },
{ id: 4, name: 'Second SetAttribute', setId: 2 }
]
}
]
});
});
});
});
};
12 changes: 6 additions & 6 deletions tests/unit/relations/ManyToManyRelation.js
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ describe('ManyToManyRelation', () => {
}
});
}).to.throwException(err => {
expect(err.message).to.equal(
expect(err.message).to.contain(
"OwnerModel.relationMappings.testRelation: Cannot find module '/not/a/path/to/a/model'"
);
});
Expand All @@ -274,7 +274,7 @@ describe('ManyToManyRelation', () => {
}
});
}).to.throwException(err => {
expect(err.message).to.equal(
expect(err.message).to.contain(
'OwnerModel.relationMappings.testRelation: join.through must be an object that describes the join table. For example: {from: "JoinTable.someId", to: "JoinTable.someOtherId"}'
);
});
Expand All @@ -296,7 +296,7 @@ describe('ManyToManyRelation', () => {
}
});
}).to.throwException(err => {
expect(err.message).to.equal(
expect(err.message).to.contain(
'OwnerModel.relationMappings.testRelation: join.through must be an object that describes the join table. For example: {from: "JoinTable.someId", to: "JoinTable.someOtherId"}'
);
});
Expand All @@ -319,7 +319,7 @@ describe('ManyToManyRelation', () => {
}
});
}).to.throwException(err => {
expect(err.message).to.equal(
expect(err.message).to.contain(
'OwnerModel.relationMappings.testRelation: join.through.from must have format JoinTable.columnName. For example "JoinTable.someId" or in case of composite key ["JoinTable.a", "JoinTable.b"].'
);
});
Expand All @@ -342,7 +342,7 @@ describe('ManyToManyRelation', () => {
}
});
}).to.throwException(err => {
expect(err.message).to.equal(
expect(err.message).to.contain(
'OwnerModel.relationMappings.testRelation: join.through.to must have format JoinTable.columnName. For example "JoinTable.someId" or in case of composite key ["JoinTable.a", "JoinTable.b"].'
);
});
Expand All @@ -365,7 +365,7 @@ describe('ManyToManyRelation', () => {
}
});
}).to.throwException(err => {
expect(err.message).to.equal(
expect(err.message).to.contain(
'OwnerModel.relationMappings.testRelation: join.through `from` and `to` must point to the same join table.'
);
});
Expand Down
4 changes: 2 additions & 2 deletions typings/objection/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,7 @@ declare namespace Objection {
}

type PartialUpdate<QM extends Model> = {
[P in keyof QM]?: QM[P] | Raw | Reference | QueryBuilder<any, any[]>
[P in keyof QM]?: QM[P] | Raw | Reference | QueryBuilder<any, any[]>;
};

interface QueryBuilderBase<QM extends Model, RM, RV> extends QueryInterface<QM, RM, RV> {
Expand Down Expand Up @@ -1556,7 +1556,7 @@ declare namespace Objection {
*/
type?: string | string[];
/**
* fallback raw string for custom formats,
* fallback raw string for custom formats,
* or formats that aren't in the standard yet
*/
format?: JsonSchemaFormatType | string;
Expand Down

0 comments on commit f328630

Please sign in to comment.