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

[tests-only][full-ci] refactor: Set chunking version 1 as default #10624

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

anon-pradip
Copy link
Contributor

@anon-pradip anon-pradip commented Nov 22, 2024

Description

This PR removes the chunkingToUse variable from the code-base which in turn makes the use of existing TUS chunk upload endpoint by default.

Related Issue

Motivation and Context

How Has This Been Tested?

  • locally
  • CI

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • 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:

@anon-pradip anon-pradip self-assigned this Nov 22, 2024
@anon-pradip anon-pradip marked this pull request as ready for review November 22, 2024 08:38
@anon-pradip anon-pradip changed the title [tests-only][full-ci] set chunking version 1 as default [tests-only][full-ci] refactor: Set chunking version 1 as default Nov 25, 2024
@anon-pradip anon-pradip force-pushed the set-chunking-version branch 2 times, most recently from c351546 to b171fbb Compare December 9, 2024 07:25
@anon-pradip anon-pradip requested a review from saw-jan December 9, 2024 09:28
@anon-pradip anon-pradip force-pushed the set-chunking-version branch 2 times, most recently from 09a1979 to 73a01af Compare December 9, 2024 11:30
}

//prepare chunking
Copy link
Contributor

Choose a reason for hiding this comment

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

what was done in this preparation?

Suggested change
//prepare chunking
//prepare chunking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$chunk → Returns array of file chunks(source file split into multiple chunks)
$chunkingId → Generates random identifier (e.g., "chunking-4567") and groups related chunks together
$result → Initializes variable to store upload response which will be updated during chunk upload process

Given using <dav-path-version> DAV path
When user "Alice" uploads file "filesForUpload/davtest.txt" to filenames based on "/davtest.txt" with all mechanisms except new chunking using the WebDAV API
And user "Alice" uploads file "filesForUpload/davtest.txt" to filenames based on "/davtest.txt" with all mechanisms except new chunking using the WebDAV API
When user "Alice" uploads file "filesForUpload/davtest.txt" to "davtest.txt" with and without chunking using the WebDAV API
Copy link
Contributor

Choose a reason for hiding this comment

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

why uploading with and without at once?

Suggested change
When user "Alice" uploads file "filesForUpload/davtest.txt" to "davtest.txt" with and without chunking using the WebDAV API
When user "Alice" uploads file "filesForUpload/davtest.txt" to "davtest.txt" with and without chunking using the WebDAV API

Copy link
Member

Choose a reason for hiding this comment

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

Scenario Outline: upload a file twice and versions are available using various chunking methods

Copy link
Member

Choose a reason for hiding this comment

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

actual scenario: #10624 (comment)

);
}
foreach ([false, true] as $chunkingUse) {
$finalDestination = $overwriteMode ? $destination : $destination . ($chunkingUse ? "-chunked" : "");
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, we should not need this.

Suggested change
$finalDestination = $overwriteMode ? $destination : $destination . ($chunkingUse ? "-chunked" : "");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use $destination directly as param in upload():

First upload (non-chunked):

  • Path: davtest.txt
  • Creates file: davtest.txt

Second upload (chunked):

  • Temporary chunks: davtest.txt-chunking-4320-2-0, davtest.txt-chunking-4320-2-1
  • Final assembled file: davtest.txt (overwrites original)
  • Creates version because of overwrite

which makes the test fail for the step:
Then the version folder of file "davtest.txt" for user "Alice" should contain "0" elements

But if we use something like $finalDestination:
First upload (non-chunked):

  • Path: davtest.txt
  • Creates file: davtest.txt

Second upload (chunked):

  • Temporary chunks: davtest.txt-chunked-chunking-8951-2-0, davtest.txt-chunked-chunking-8951-2-1
  • Final assembled file: davtest.txt-chunked
  • No version created because files are separate

Copy link
Member

Choose a reason for hiding this comment

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

if the mentioned step fails, we have to look at what is happening and maybe scenario is not clear.
as per the scenario, I thought the the different upload methods to create same file will not creat file versions.

Copy link
Member

Choose a reason for hiding this comment

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

let's have a look together

Copy link
Contributor Author

@anon-pradip anon-pradip Dec 12, 2024

Choose a reason for hiding this comment

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

After having closer look together with @saw-jan:

  • refactored the scenarios from:
 Scenario Outline: upload file and no version is available using various chunking methods (except new chunking),
 Scenario Outline: upload a file twice and versions are available using various chunking methods (except new chunking)

to:

Scenario Outline: no version is available while uploading a file with multiple chunks
Scenario Outline: versions are available while uploading a file twice with multiple chunks

respectively for providing more clear sense.

  • removed uploadWithAllMechanisms as it will be unused.
  • changed return types to ResponseInterface for consistency and better error handling.

Copy link
Member

@saw-jan saw-jan left a comment

Choose a reason for hiding this comment

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

👍

@saw-jan saw-jan merged commit fbf49a7 into master Dec 13, 2024
4 checks passed
@saw-jan saw-jan deleted the set-chunking-version branch December 13, 2024 05:40
ownclouders pushed a commit that referenced this pull request Dec 13, 2024
[tests-only][full-ci] refactor: Set chunking version 1 as default
ownclouders pushed a commit that referenced this pull request Dec 14, 2024
[tests-only][full-ci] refactor: Set chunking version 1 as default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QA] Set chunking version 1 as default
6 participants