X Tutup
Skip to content

extend share_external name column length to 255#37835

Merged
phil-davis merged 1 commit intomasterfrom
extend_share_external_name
Aug 24, 2020
Merged

extend share_external name column length to 255#37835
phil-davis merged 1 commit intomasterfrom
extend_share_external_name

Conversation

@karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented Aug 23, 2020 •
edited
Loading

Description

Extend share_external name column length to 255

Related Issue

Motivation and Context

oc_filecache allows filename up to 250 character, federated share file name should support same limits.

How Has This Been Tested?

  • Manually executed federation and checked the column lenghth from db.
  • There is an acceptance test covers this scenario.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@karakayasemi karakayasemi force-pushed the extend_share_external_name branch from de8bba8 to ce58b99 Compare August 23, 2020 12:44
@karakayasemi karakayasemi self-assigned this Aug 23, 2020
@karakayasemi karakayasemi added this to the development milestone Aug 23, 2020
@phil-davis
Copy link
Contributor

phil-davis commented Aug 23, 2020

@karakayasemi fails code-style https://drone.owncloud.com/owncloud/core/26430/2/6

@phil-davis
Copy link
Contributor

phil-davis commented Aug 23, 2020

Also I expect that you will need to bump the version in version.php so that the migration will run for people (and CI) before the changed acceptance test.

$OC_Version = [10, 5, 1, 1];

// The human readable string
$OC_VersionString = '10.5.1alpha2';

@karakayasemi karakayasemi force-pushed the extend_share_external_name branch from ce58b99 to 9614d64 Compare August 23, 2020 13:22
@karakayasemi
Copy link
Contributor Author

karakayasemi commented Aug 23, 2020 •
edited
Loading

@phil-davis Thank you for help, I fixed the code style problems. I already bumped the files_sharing app version. I guess, it should be enough for the migration. If CI will not pass, I will try to bump core version.
I bumped core version as you suggested. 😃

@karakayasemi karakayasemi force-pushed the extend_share_external_name branch from 9614d64 to 84804dd Compare August 23, 2020 14:02
phil-davis
phil-davis reviewed Aug 23, 2020
View reviewed changes
@karakayasemi karakayasemi force-pushed the extend_share_external_name branch from 84804dd to b14c3aa Compare August 23, 2020 21:27
This was referenced Aug 24, 2020
Closed
Merged
@phil-davis phil-davis force-pushed the extend_share_external_name branch from b14c3aa to 4d3f892 Compare August 24, 2020 04:24
@phil-davis
Copy link
Contributor

phil-davis commented Aug 24, 2020

PR #37837 add acceptance test support ffor tags like skipOnFedOcV10.3 so that it is possible to skip scenarios that are fixes that are known not to work against an old federated server.

I rebased and added the new skip tags.

@owncloud owncloud deleted a comment from update-docs bot Aug 24, 2020
@phil-davis
Copy link
Contributor

phil-davis commented Aug 24, 2020

CI fails https://drone.owncloud.com/owncloud/core/26444/132/19 because the federated server is only running daily-master-qa - it does not have this code in it.

PR #37838 is fixing that so that the federated server will also run the code from the PR.

This was referenced Aug 24, 2020
Closed
Merged
jvillafanez
jvillafanez reviewed Aug 24, 2020
View reviewed changes
apps/files_sharing/appinfo/Migrations/Version20200823121322.php
if ($schema->hasTable("${prefix}share_external")) {
$table = $schema->getTable("{$prefix}share_external");
$nameColumn = $table->getColumn('name');
if ($nameColumn && $nameColumn->getLength() !== 255) {
Copy link
Member

@jvillafanez jvillafanez Aug 24, 2020

Choose a reason for hiding this comment

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

If this should match the filecache's name column, the current length is 250 (at least in my environment)

Copy link
Contributor Author

@karakayasemi karakayasemi Aug 24, 2020

Choose a reason for hiding this comment

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

You are right, but share provider adds '/' characters to the name before insterting share to the database. So, I intentionally selected the length as 255 to create buffer for this kind of situations.

@phil-davis phil-davis force-pushed the extend_share_external_name branch from 4d3f892 to f39c52a Compare August 24, 2020 12:06
@phil-davis phil-davis self-requested a review August 24, 2020 12:06
@phil-davis
Copy link
Contributor

phil-davis commented Aug 24, 2020

CI fails https://drone.owncloud.com/owncloud/core/26444/132/19 because the federated server is only running daily-master-qa - it does not have this code in it.

PR #37838 is fixing that so that the federated server will also run the code from the PR.

I cherry-picked that fed-server CI .drone.star stuff into this PR. It should pass now.

@phil-davis
Copy link
Contributor

phil-davis commented Aug 24, 2020

codecov results have got "stuck". Actually this PR looks fine and all drone has passed. I will sort out merging it.

@phil-davis phil-davis force-pushed the extend_share_external_name branch from f39c52a to 0367c41 Compare August 24, 2020 13:16
@phil-davis phil-davis merged commit 6423842 into master Aug 24, 2020
@phil-davis phil-davis deleted the extend_share_external_name branch August 24, 2020 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

@phil-davis phil-davis phil-davis approved these changes

@mrow4a mrow4a Awaiting requested review from mrow4a

@VicDeo VicDeo Awaiting requested review from VicDeo

Assignees

@karakayasemi karakayasemi

Labels

3 - To Review

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

Cannot receive a federated share of a file with name > 64 characters

4 participants

@karakayasemi @phil-davis @VicDeo @jvillafanez
X Tutup