Skip to content
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

Implement "RESTRICT" foreign key constraint in Wt::Dbo module #161

Merged
merged 3 commits into from
Apr 9, 2020
Merged

Implement "RESTRICT" foreign key constraint in Wt::Dbo module #161

merged 3 commits into from
Apr 9, 2020

Conversation

ekondayan
Copy link
Contributor

The RESTRICT foreign key constraint is supported by all the major databases.
It is a must have feature for quite a lot of database schemes.
It's implementation in the Wt::Dbo module is very straight forward process, so there is no apparent reason no to do so.

Tested:

  1. Wt compiles successfully
  2. SQLite3 - SQL generation for the tables works as expected

* Add Wt::Dbo::Impl::FKOnUpdateRestrict  = 0x08 and Wt::Dbo::Impl::FKOnDeleteRestrict = 0x40
* Change the bit flags of the other constants so the newly added ones fit nicely
* Add SQL code generation
@emweb
Copy link
Collaborator

emweb commented Apr 9, 2020

Thanks for your contribution. Your changes look good, and I know it's fairly trivial, but what would be better is if we had some unit tests, so if you can manage to write some unit tests that would be ideal. If you can check it with Sqlite3, we can make sure it works with all of the other ones.

I suppose this is a case of checking Session::tableCreationSql() and seeing if Session::createTables() works?

Regards,
Roel

@ekondayan
Copy link
Contributor Author

I've written the test, but I do not know how to include it into the framework.
Should I put into DboTest.C or should I create a new file DboTest8.C?
Could you guide me how to do it?

* Add stand alone unit test
* Code format - FKNotNull's assignment to be aligned with others
* Remove Sqlite property "show-queries"
@emweb
Copy link
Collaborator

emweb commented Apr 9, 2020

That's a good start, I can take it from there.

Regards,
Roel

@emweb emweb merged commit baf2477 into emweb:master Apr 9, 2020
emweb pushed a commit that referenced this pull request Apr 9, 2020
@ekondayan
Copy link
Contributor Author

ekondayan commented Apr 9, 2020

There is something that bothers me a bit.
File Wt/Dbo/Session.C Lines: 912
The if statement check the variable haveSupportUpdateCascade_ which is set here

void Session::initSchema() const
{
...
haveSupportUpdateCascade_ = conn->supportUpdateCascade();
...
}

Do we need to add another variable haveSupportUpdateRestrict_ next to haveSupportUpdateCascade_

void Session::initSchema() const
{
...
haveSupportUpdateCascade_ = conn->supportUpdateCascade();
haveSupportUpdateRestrict_ = conn->supportUpdateRestrict();
...
}

and change the if statement to

std::string Session::constraintString(Impl::MappingInfo *mapping, ...
{
...
else if (field.fkConstraints() & Impl::FKOnUpdateRestrict
&& haveSupportUpdateRestrict_)
...
}

Or since all databases support the RESTRICT constraint, checking haveSupportUpdateRestrict_ is useless?
Or to leave it like it is?

@emweb
Copy link
Collaborator

emweb commented Apr 9, 2020

Good observation. It appears that that check is there for Oracle. I'm still checking to see what Oracle supports. The implementation seems to suggest that ON UPDATE is just not supported in general, since it is also used to disable ON UPDATE SET NULL, so it's kind of poorly named.

In any case, I don't think it's necessary to introduce a supportUpdateRestrict().

Regards,
Roel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants