-
Notifications
You must be signed in to change notification settings - Fork 183
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
Feature add foreign key on update on delete trigger config #370
Feature add foreign key on update on delete trigger config #370
Conversation
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
08f6788
to
67345bd
Compare
@slnode test please |
I'm not sure but some test in datasource juggler in windows node 6.x failed, is it related with my changes ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HugoPoi Sorry for the late reply. I left a comment and triggered a new build for CI.
}); | ||
rawConstraints.push(match); | ||
} | ||
} while (match != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the while
loop is a little bit risky to me, what if the statement doesn't contain the constraint? Maybe change the logic to stop when reaches the last line of the statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the if
statement line 54 in the while
protect for no match in the CREATE TABLE
, I think the tricky part is when someone rework my code later, because the iteration over the Regex is not obvious. Maybe add some comment before the line 52 to explain the iteration on the multiple match with RegExp object internal pointer.
Or is there a other way to iterate with complex RegExp ?
@slnode test please |
@jannyHou, the CI is all green. Do you think this PR is good to merge? Thanks. |
67345bd
to
24fe635
Compare
@slnode test please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HugoPoi Thank you for the contribution 🎉 !!
LGTM. The travis failure is not related to your PR. I will land it when the jenkins tests pass.
Description
Add possibility to configure the foreign key constraint with 2 optional triggers.
onUpdate
foreign key settingsonDelete
foreign key settingsRelated issues
Checklist
guide