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

enhancement - REFERENCES and FOREIGN KEY in XML notation #5756

Closed
Elodrin opened this issue Nov 7, 2013 · 13 comments
Closed

enhancement - REFERENCES and FOREIGN KEY in XML notation #5756

Elodrin opened this issue Nov 7, 2013 · 13 comments

Comments

@Elodrin
Copy link

Elodrin commented Nov 7, 2013

Hi,

I am programming a chat-app and need some database-tables for that. I wanted to use REFERENCES and PRIMARY/FOREIGN KEY relations but didn't found anything about that. In ownCloud IRC I got bad news. One of the most powerful features of a database was not implemented because ownCloud wants to support older versions of sqlite...
Please think about it. Is it realy necessary? Maybe it is possible to implement this feature with a securety-check, so when using this feature owncloud must run on a current database version.

Greetings

@wshadow
Copy link

wshadow commented Nov 11, 2013

I don't think sqlite is the problem here. I did some reading on this, and the issue seems to be the support for MyIsam (if using mysql). We had some discussions about this since OC3. You can look into the mailinglist archives for more information on this.

Your question is still valid though. And I'd also like to see owncloud use more powerful database features.

@karlitschek
Copy link
Contributor

One of the design goal of ownCloud is to Run on as many systems as possible. We don't plan to break compatibility just for fun. And I'm still waiting to see an application that can't be implemented in a cross database compatible way

@VicDeo
Copy link
Member

VicDeo commented Nov 11, 2013

@karlitschek Foreign keys are supported with sqlite since 3.6.19 released on 14 October 2009
For reference: PHP 5.3.2 was released on 4 March 2010 so I wonder too why we can't use cascade delete/update instead of juggling with SQL inside PHP

@wshadow
Copy link

wshadow commented Nov 11, 2013

@VicDeo the problem is mysql - see http://dev.mysql.com/doc/refman/5.0/en/ansi-diff-foreign-keys.html

@karlitschek what do you think about apps requiring some specific software/backends. e.g. allowing the app to check which database is used and refusing to be installed if it is incompatible? (similar to requiring an OC-core version)

@karlitschek
Copy link
Contributor

@VicDeo we have to check if this version is shipped with all distributions that we support. It also has to work in all databases that we Support in a completely compatible Way. And it has to be supported by MDB2 and doctrine so that schema migration works always. And we would need tests for that. We already have more then enough unhappy users with broken databases and this woudn't make things easier.

@karlitschek
Copy link
Contributor

@wshadow this would be a bad idea. We have to stay compatible.

@VicDeo
Copy link
Member

VicDeo commented Nov 11, 2013

In reality foreign keys are designed to keep the database data consistent by means of Db engine itself. Just as it is stated by the link provided by @wshadow above:

Assuming proper design of the relationships, foreign key constraints make it more difficult for a programmer to introduce an inconsistency into the database.

@wshadow So, mandatory InnoDB storage is considered to be a real stumbling block for using constrains, correct?

@karlitschek I probably would never say something like 'We need to reimplement everything using a technology X' but rather "let's consider some X in future as it is safe and time-saving"

From my POV it is worth to be discussed for OC7 where MDB backward compatibility is not essential (IIRC we don't support direct migrations among nonsequential versions anyway)
Doctrine supports them among a lot of other powerful things we are not using like collections and ORM.
Good old Ubuntu 10.04 has sqlite 3.6.22

@wshadow
Copy link

wshadow commented Nov 12, 2013

@VicDeo you are correct. So far we allow any storage engine to be used in mysql. If we start using (and relying upon) foreign keys we force admins to switch to InnoDB as their storage engine, if they use mysql.
In other words: If you have code that makes use of foreign keys and expects that the db supports this feature, BUT you still use mysql without InnoDB, you risk to get a corrupted database. Note that this does not necessarily mean, that the code does not function anymore. It may just clutter up your db.

If there is serious interest in discussing this further I'd be willing to write up some pros and cons about using foreign keys in owncloud (-apps). However, this will take a bit, as I have some other projects that I'm working on atm.

@sureshprakash
Copy link

I could understand that compatibility is an issue. But, it's really difficult for some developers to write code to delete the tuples in every table instead of a simple 'ON DELETE CASCADE' command. It is really difficult and a time consuming task for developers.

@wshadow
Copy link

wshadow commented Jan 7, 2014

@sureshprakash I'm on your side regarding that. Imagine you reference a table from a different app (e.g. a user); it's going to be a pain to notice all the delete and update operations on such an "app external" table.

@jancborchardt
Copy link
Member

I’m closing this issue because it has been inactive for a few months. This probably means that the issue is not reproducible or it has been fixed in a newer version.

Please open a new issue if you still encounter this problem with the latest stable version and then please use the issue template. You can also contribute directly by providing a patch – see the developer manual. :)

Thank you!

@wshadow
Copy link

wshadow commented Jul 16, 2014

@jancborchardt when closing the issue you could at least use a fitting text block ;). This is about an enhancement (I'd even go as far as to call it a feature request) and therefore we should use a different text block.
For this issue I suggest sth. like: "We don't want to add this feature because all apps have to be compatible with MyISAM".

@jancborchardt
Copy link
Member

@wshadow sorry about the missing clarity. :) I’m cleaning up old issues and since Frank stated that we can’t do this because we have to stay compatible, this should be closed. Hope that clears it up.
And thank you for any enhancement requests! :) We really value it, but of course always have to decide whether it’s feasible or not.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants