Skip to content

Use string for storing a OCM remote id - #24247

Merged
juliusknorr merged 8 commits into
masterfrom
bugfix/noid/ocm-providerId-string
Dec 9, 2020
Merged

Use string for storing a OCM remote id#24247
juliusknorr merged 8 commits into
masterfrom
bugfix/noid/ocm-providerId-string

Conversation

@juliusknorr

@juliusknorrjuliusknorr commented Nov 20, 2020

Copy link
Copy Markdown
Member

This is required to make sure we are compatible with Federated Sharing OCM as proposed in cs3org/OCM-API#37 providerId is a string.

While #9345 added general support for this our backend does only allow integers to be stored as a providerId, since we still have integer values in our database

  • Move files_sharing to database migration (only contains oc_share_external)
  • Migrate share_external to use bigint
  • Migrate federated_reshares to use bigint
  • Move parameter types of remoteId usages to string
  • Migrate remoteId columns to string
  • Migrate default of remoteId columns to be an empty string instead of -1
    • This is the change I hesitated a bit since I don't know the federation code well enough to dare estimating how things might break. However i could not find any place where this value would be checked and tests seem to be still fine with it.

@juliusknorrjuliusknorr added this to the Nextcloud 21 milestone Nov 20, 2020
@juliusknorr
juliusknorrforce-pushed the bugfix/noid/ocm-providerId-string branch 2 times, most recently from eae4899 to 33694e5CompareNovember 20, 2020 14:50
@juliusknorr
juliusknorrforce-pushed the bugfix/noid/ocm-providerId-string branch 5 times, most recently from 497f888 to 061b048CompareDecember 4, 2020 13:13
@juliusknorrjuliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 4, 2020
@juliusknorr
juliusknorr marked this pull request as ready for review December 4, 2020 13:33

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

$remoteIdColumn = $table->getColumn('remote_id');
if ($remoteIdColumn && $remoteIdColumn->getType()->getName() !== Types::STRING) {
$remoteIdColumn->setNotnull(false);
$remoteIdColumn->setType(Type::getType(Types::STRING));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if our "friend" likes this too much. But the test seems to say yes.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some "friends" actually did a similar migration, this came up as a part of #23044

Comment threadapps/federatedfilesharing/lib/Migration/Version1011Date20201120125158.php Outdated
Signed-off-by: Julius Härtl <jus@bitgrid.net>
… share_external
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
…registered yet
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
juliusknorrforce-pushed the bugfix/noid/ocm-providerId-string branch from 061b048 to b732604CompareDecember 8, 2020 15:06
@faily-bot

faily-botBot commented Dec 8, 2020

Copy link
Copy Markdown

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 306: failure

sqlite

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There were 4 failures:
1) Test\Preview\BitmapTest::testGetThumbnail with data set #0 (-88, -74)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
2) Test\Preview\BitmapTest::testGetThumbnail with data set #1 (66, 86)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
3) Test\Preview\BitmapTest::testGetThumbnail with data set #2 (-36, 60)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
4) Test\Preview\BitmapTest::testGetThumbnail with data set #3 (81, -76)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

mariadb10.4-php7.4

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There were 4 failures:
1) Test\Preview\BitmapTest::testGetThumbnail with data set #0 (-56, -100)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
2) Test\Preview\BitmapTest::testGetThumbnail with data set #1 (72, 43)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
3) Test\Preview\BitmapTest::testGetThumbnail with data set #2 (-22, 9)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
4) Test\Preview\BitmapTest::testGetThumbnail with data set #3 (86, -8)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

mysql8.0-php7.4

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There were 4 failures:
1) Test\Preview\BitmapTest::testGetThumbnail with data set #0 (-96, -16)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
2) Test\Preview\BitmapTest::testGetThumbnail with data set #1 (8, 26)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
3) Test\Preview\BitmapTest::testGetThumbnail with data set #2 (-44, 91)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
4) Test\Preview\BitmapTest::testGetThumbnail with data set #3 (47, -16)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

postgres11-php7.4

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There were 4 failures:
1) Test\Preview\BitmapTest::testGetThumbnail with data set #0 (-91, -12)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
2) Test\Preview\BitmapTest::testGetThumbnail with data set #1 (33, 79)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
3) Test\Preview\BitmapTest::testGetThumbnail with data set #2 (-18, 8)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105
4) Test\Preview\BitmapTest::testGetThumbnail with data set #3 (64, -100)
Failed asserting that null is not equal to false.
/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

@juliusknorrjuliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 8, 2020
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Failures unrelated, erroring on master as well.

@juliusknorr
juliusknorr merged commit a0444bc into masterDec 9, 2020
@juliusknorr
juliusknorr deleted the bugfix/noid/ocm-providerId-string branch December 9, 2020 16:26
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

/backport to stable20

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: federation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@juliusknorr@nickvergessen@ChristophWurst