Reverted commit 54a5f938f99 because of Bug #23174

[Cybso]

Reverted commit 54a5f93 from @nickvergessen to fix bug #23174 which breaks upgrades to 18.0.10.

Maybe it would be better to fix the migration of oc_flow_operations instead, but I'm not familiar with this part of the code.

fix #23174

[nickvergessen]

Yes need to fix flow instead @blizzz

[Cybso]

@nickvergessen @blizzz I've reverted the revert and fixed the migration instead. Since I assume that the column 'entity' should never become Null when added, I've changed 'notnull' to FALSE instead of removing the empty default value. Is this a valid fix for the problem?

A comment on the original test: would it not be better to differentiate between NotNull and NotBlank?

[nickvergessen]

Well empty strings are saved as Null in Oracle, so having an NotNull with default '' is invalid and that's why we prevent it

[Cybso]

Well, that's bad. While MariaDB obviously accepts adding "not null" columns without explicit default, SQLite does not:

Mariadb:

MariaDB [nextclouddev]> create table foobar (id int not null primary key);
Query OK, 0 rows affected (0.196 sec)

MariaDB [nextclouddev]> insert into foobar values (1);
Query OK, 1 row affected (0.030 sec)

MariaDB [nextclouddev]> insert into foobar values (2);
Query OK, 1 row affected (0.178 sec)

MariaDB [nextclouddev]> alter table foobar add column x char(10) not null;
Query OK, 0 rows affected (0.149 sec)
Records: 0  Duplicates: 0  Warnings: 0

MariaDB [nextclouddev]> select * from foobar;
+----+---+
| id | x |
+----+---+
|  1 |   |
|  2 |   |
+----+---+
2 rows in set (0.001 sec)

SQLite:

sqlite> create table foobar (id int not null primary key);
sqlite> insert into foobar values (1);
sqlite> insert into foobar values (2);
sqlite> alter table foobar add column x char(10) not null;
Error: Cannot add a NOT NULL column with default value NULL

[blizzz]

@Cybso could you squash the commits, please?

[Cybso]

@blizzz I'm still not very familiar with the extended git features. I've used "git rebase -i HEAD~2" to squash the second commit with the first one, but when I try to push this to Github, I get the following error:

To github.com:Cybso/server.git
 ! [rejected]              23174-revert-commit-from-54a5f938f99 -> 23174-revert-commit-from-54a5f938f99 (non-fast-forward)
error: Fehler beim Versenden einiger Referenzen nach 'git@github.com:Cybso/server.git

Can you give me I hint how to fix this?

[ChristophWurst]

Can you give me I hint how to fix this?

You need --force ;)

[nickvergessen]

We need a follow up migration which fixes existing instances?

[Cybso]

I think it would be nice to have matching database definition across all instances, but not exactly required. The module won't write any NULL values into this column, so at least for MySQL and SQLite it doesn't matter if it's true or false (cannot speak for Oracle). An alternative implementation would be to add a second call which updates the database definition to "NotNull = true" and removes the default value after the table has been extended.

@blizzz, what's your opinion?

[blizzz]

Consistency and an expected state is to be preferred, otherwise we indeed may run into trouble in future changes. So, yes, a migration step would be ideal to add.

[Cybso]

Sorry, I've tried, but at the moment I do not find the time to implement this second migration. Can someone take over?

[nickvergessen]

I will

[nickvergessen]

Done in #23882