Skip to content

Commit c4e1fd0

Browse files
jedwards1211jedwards1211
committed
fix: non-behavior-preserving bug
basically ```js model.create({}).then(instance => { if (previousInstance) { return previousInstance[\`set\${_.upperFirst(model.name)}\`](instance).then(() => { previousInstance = instance; }); } previousInstance = b = instance; }); ``` Was getting converted to ```js const instance = await model.create({}); if (previousInstance) { await previousInstance[\`set\${_.upperFirst(model.name)}\`](instance); previousInstance = instance; } previousInstance = b = instance; ``` Which is buggy because the if statement shouldn't fall through. The fix is adds a `return` after `previousInstance = instance`.
1 parent c5cb87d commit c4e1fd0

File tree

5 files changed

+220
-9
lines changed

5 files changed

+220
-9
lines changed

‎src/util/convertConditionalReturns.ts

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,7 @@ import * as t from '@babel/types'
22
import { NodePath } from '@babel/traverse'
33
import replaceWithStatements from './replaceWithStatements'
44
import removeRestOfBlockStatement from './removeRestOfBlockStatement'
5-
6-
function isLastStatementInBlock(path: NodePath<any>): boolean {
7-
const { parentPath } = path
8-
if (!parentPath.isBlockStatement()) return true
9-
const body = (parentPath as NodePath<t.BlockStatement>).get('body')
10-
return (path as NodePath<any>) === body[body.length - 1]
11-
}
5+
import { isLastStatementInBlock } from './predicates'
126

137
function hasReturn(path: NodePath<t.Statement>): boolean {
148
if (path.isReturnStatement()) return true

‎src/util/hasReturnStatements.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import * as t from '@babel/types'
2+
import { NodePath } from '@babel/traverse'
3+
4+
export default function hasReturnStatements(
5+
path: NodePath<t.BlockStatement>
6+
): boolean {
7+
let found = false
8+
path.traverse({
9+
ReturnStatement(path: NodePath<t.ReturnStatement>) {
10+
found = true
11+
path.stop()
12+
},
13+
Function(path: NodePath<t.Function>) {
14+
path.skip()
15+
},
16+
})
17+
return found
18+
}

‎src/util/predicates.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,3 +132,18 @@ export function isInTryBlock(path: NodePath<any>): boolean {
132132
}
133133
return false
134134
}
135+
136+
export function isLastStatementInBlock(path: NodePath<any>): boolean {
137+
const { parentPath } = path
138+
if (!parentPath.isBlockStatement()) return true
139+
const body = (parentPath as NodePath<t.BlockStatement>).get('body')
140+
return (path as NodePath<any>) === body[body.length - 1]
141+
}
142+
143+
export function isLastStatementInFunction(path: NodePath<any>): boolean {
144+
while (!path.isFunction()) {
145+
if (!isLastStatementInBlock(path)) return false
146+
path = path.parentPath
147+
}
148+
return true
149+
}

‎src/util/replaceLink.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@ import {
66
isIdentifierAssignmentExpression,
77
isIdentifierDeclarator,
88
isInTryBlock,
9+
isLastStatementInFunction,
910
} from './predicates'
1011
import renameBoundIdentifiers from './renameBoundIdentifiers'
1112
import unboundIdentifier from './unboundIdentifier'
1213
import replaceReturnStatements from './replaceReturnStatements'
1314
import { awaited } from './builders'
1415
import insertStatementsBefore from './insertStatementsBefore'
1516
import replaceWithStatements from './replaceWithStatements'
17+
import hasReturnStatements from './hasReturnStatements'
1618

1719
function findReplaceTarget<T extends t.Node>(link: NodePath<T>): NodePath<any> {
1820
const { parentPath } = link
@@ -83,15 +85,22 @@ export default function replaceLink<T extends t.Expression | t.BlockStatement>(
8385
target.parentPath.isBlockParent() &&
8486
!target.parentPath.isBlockStatement()
8587
) {
86-
// return replaceBlockParent(target, t.blockStatement(replacement.node.body))
8788
return replaceWithStatements(target, replacement.node.body) as any
8889
} else if (target.isReturnStatement()) {
8990
if (isInTryBlock(target)) {
9091
replaceReturnStatements(replacement, argument =>
9192
t.returnStatement(awaited(argument))
9293
)
9394
}
94-
return replaceWithStatements(target, replacement.node.body) as any
95+
if (
96+
hasReturnStatements(replacement) ||
97+
isLastStatementInFunction(target)
98+
) {
99+
return replaceWithStatements(target, replacement.node.body) as any
100+
} else {
101+
;(target as NodePath<t.ReturnStatement>).get('argument').remove()
102+
return insertStatementsBefore(target, replacement.node.body) as any
103+
}
95104
} else if (target.isExpressionStatement()) {
96105
replaceReturnStatements(replacement, argument =>
97106
t.expressionStatement(awaited(argument))

‎test/fixtures/bugs_findAll.test.ts

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
export const input = `
2+
it('should support many levels of belongsTo', function() {
3+
const A = this.sequelize.define('a', {}),
4+
B = this.sequelize.define('b', {}),
5+
C = this.sequelize.define('c', {}),
6+
D = this.sequelize.define('d', {}),
7+
E = this.sequelize.define('e', {}),
8+
F = this.sequelize.define('f', {}),
9+
G = this.sequelize.define('g', {}),
10+
H = this.sequelize.define('h', {});
11+
12+
A.belongsTo(B);
13+
B.belongsTo(C);
14+
C.belongsTo(D);
15+
D.belongsTo(E);
16+
E.belongsTo(F);
17+
F.belongsTo(G);
18+
G.belongsTo(H);
19+
20+
return this.sequelize.sync({ force: true }).then(() => {
21+
return Promise.all([A.bulkCreate([
22+
{},
23+
{},
24+
{},
25+
{},
26+
{},
27+
{},
28+
{},
29+
{}
30+
]).then(() => {
31+
return A.findAll();
32+
}), (function(singles) {
33+
let promise = Promise.resolve(),
34+
previousInstance,
35+
b;
36+
37+
singles.forEach(model => {
38+
promise = promise.then(() => {
39+
return model.create({}).then(instance => {
40+
if (previousInstance) {
41+
return previousInstance[\`set\${_.upperFirst(model.name)}\`](instance).then(() => {
42+
previousInstance = instance;
43+
});
44+
}
45+
previousInstance = b = instance;
46+
});
47+
});
48+
});
49+
50+
promise = promise.then(() => {
51+
return b;
52+
});
53+
54+
return promise;
55+
})([B, C, D, E, F, G, H])]).then(([as, b]) => {
56+
return Promise.all(as.map(a => {
57+
return a.setB(b);
58+
}));
59+
}).then(() => {
60+
return A.findAll({
61+
include: [
62+
{ model: B, include: [
63+
{ model: C, include: [
64+
{ model: D, include: [
65+
{ model: E, include: [
66+
{ model: F, include: [
67+
{ model: G, include: [
68+
{ model: H }
69+
] }
70+
] }
71+
] }
72+
] }
73+
] }
74+
] }
75+
]
76+
}).then(as => {
77+
expect(as.length).to.be.ok;
78+
79+
as.forEach(a => {
80+
expect(a.b.c.d.e.f.g.h).to.be.ok;
81+
});
82+
});
83+
});
84+
});
85+
});
86+
`
87+
88+
export const options = {}
89+
90+
export const expected = `
91+
it('should support many levels of belongsTo', async function() {
92+
const A = this.sequelize.define('a', {}),
93+
B = this.sequelize.define('b', {}),
94+
C = this.sequelize.define('c', {}),
95+
D = this.sequelize.define('d', {}),
96+
E = this.sequelize.define('e', {}),
97+
F = this.sequelize.define('f', {}),
98+
G = this.sequelize.define('g', {}),
99+
H = this.sequelize.define('h', {});
100+
101+
A.belongsTo(B);
102+
B.belongsTo(C);
103+
C.belongsTo(D);
104+
D.belongsTo(E);
105+
E.belongsTo(F);
106+
F.belongsTo(G);
107+
G.belongsTo(H);
108+
109+
await this.sequelize.sync({ force: true });
110+
111+
await A.bulkCreate([
112+
{},
113+
{},
114+
{},
115+
{},
116+
{},
117+
{},
118+
{},
119+
{}
120+
]);
121+
122+
const [as0, b] = await Promise.all([await A.findAll(), (function(singles) {
123+
let promise = Promise.resolve(),
124+
previousInstance,
125+
b;
126+
127+
singles.forEach(model => {
128+
promise = (async () => {
129+
await promise;
130+
const instance = await model.create({});
131+
if (previousInstance) {
132+
await previousInstance[\`set\${_.upperFirst(model.name)}\`](instance);
133+
previousInstance = instance;
134+
return
135+
}
136+
previousInstance = b = instance;
137+
})();
138+
});
139+
140+
promise = promise.then(() => {
141+
return b;
142+
});
143+
144+
return promise;
145+
})([B, C, D, E, F, G, H])]);
146+
147+
await Promise.all(as0.map(a => {
148+
return a.setB(b);
149+
}));
150+
151+
const as = await A.findAll({
152+
include: [
153+
{ model: B, include: [
154+
{ model: C, include: [
155+
{ model: D, include: [
156+
{ model: E, include: [
157+
{ model: F, include: [
158+
{ model: G, include: [
159+
{ model: H }
160+
] }
161+
] }
162+
] }
163+
] }
164+
] }
165+
] }
166+
]
167+
});
168+
169+
expect(as.length).to.be.ok;
170+
171+
as.forEach(a => {
172+
expect(a.b.c.d.e.f.g.h).to.be.ok;
173+
});
174+
});
175+
`

0 commit comments

Comments
 (0)