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

Complex webdav properties support #37314

Merged
merged 3 commits into from
May 5, 2020
Merged

Complex webdav properties support #37314

merged 3 commits into from
May 5, 2020

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Apr 27, 2020

  • requires at least minor version bump for the dav app

Description

Store webdav property type along with the value

Related Issue

Fixes #32670
Fixes #37027

Motivation and Context

Proper Complex webdav properties support

How Has This Been Tested?

acceptance tests
also see #32670 (comment)

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

@VicDeo VicDeo added this to the development milestone Apr 27, 2020
@VicDeo VicDeo self-assigned this Apr 27, 2020
@phil-davis
Copy link
Contributor

@VicDeo I expect that you will need to adjust acceptance test scenario https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties/setFileProperties.feature#L23 to check for the now-correct value being returned.

@VicDeo
Copy link
Member Author

VicDeo commented Apr 27, 2020

@phil-davis thanks. I want to check whether it fails anything else.

@VicDeo VicDeo force-pushed the bugfix/32670 branch 6 times, most recently from 18e24f4 to af66885 Compare April 27, 2020 16:05
@DeepDiver1975
Copy link
Member

Please keep in mind that this table is really hugh on some installations - schema change will take forever .....

@VicDeo
Copy link
Member Author

VicDeo commented Apr 29, 2020

@DeepDiver1975 yes, but it is possible to add these fields in advance before upgrading the code.
I wrote the migration in a way that it does nothing when the field exists already

@VicDeo
Copy link
Member Author

VicDeo commented Apr 29, 2020

@phil-davis I updated the acceptance test as it parsed complex values incorrectly.

@VicDeo VicDeo force-pushed the bugfix/32670 branch 4 times, most recently from 296e609 to 7c37dac Compare April 30, 2020 12:02
@owncloud owncloud deleted a comment from codecov bot Apr 30, 2020
@owncloud owncloud deleted a comment from codecov bot Apr 30, 2020
@owncloud owncloud deleted a comment from codecov bot Apr 30, 2020
@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #37314 into master will decrease coverage by 0.00%.
The diff coverage is 67.30%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37314      +/-   ##
============================================
- Coverage     64.55%   64.54%   -0.01%     
- Complexity    19207    19216       +9     
============================================
  Files          1267     1268       +1     
  Lines         75065    75097      +32     
  Branches       1331     1331              
============================================
+ Hits          48458    48475      +17     
- Misses        26215    26230      +15     
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.70% <67.30%> (-0.01%) 19216.00 <10.00> (+9.00) ⬇️
Impacted Files Coverage Δ Complexity Δ
...s/dav/appinfo/Migrations/Version20200427142541.php 0.00% <0.00%> (ø) 3.00 <3.00> (?)
...ps/dav/lib/DAV/AbstractCustomPropertiesBackend.php 93.15% <77.77%> (-5.07%) 25.00 <7.00> (+7.00) ⬇️
apps/dav/lib/DAV/FileCustomPropertiesBackend.php 88.23% <83.33%> (+0.11%) 30.00 <0.00> (-1.00) ⬆️
apps/dav/lib/DAV/MiscCustomPropertiesBackend.php 72.72% <100.00%> (+1.57%) 12.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e38bb87...3f38112. Read the comment docs.

@micbar
Copy link
Contributor

micbar commented May 4, 2020

@pmaier1 Let us consider to include it in 10.5

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

Looks OK in acceptance tests.

Needs a couple of unit tests adjusted for dav 0.6.0

@jvillafanez
Copy link
Member

Code changes look fine.
There is a deleted comment about Oracle and php 7.4. I'm not sure if we have tests with that setup or if this has been tested.

@phil-davis
Copy link
Contributor

There is a deleted comment about Oracle and php 7.4. I'm not sure if we have tests with that setup or if this has been tested.

Is it related to #37312 ? and issue #37026 ? That was an intermittent problem with restoring checksums from trashbin with Oracle. (maybe not related)

@jvillafanez
Copy link
Member

Is it related to #37312 ? and issue #37026 ? That was an intermittent problem with restoring checksums from trashbin with Oracle. (maybe not related)

This is the comment https://github.com/owncloud/core/pull/37314/files#diff-d9adeff76c4cf2852e19574ce708c588L190-L192
It seems unrelated to the checksum problem mentioned

@VicDeo
Copy link
Member Author

VicDeo commented May 4, 2020

@jvillafanez

This is the comment https://github.com/owncloud/core/pull/37314/files#diff-d9adeff76c4cf2852e19574ce708c588L190-L192
It seems unrelated to the checksum problem mentioned

This PR fixes exactly that FixMe: a webdav property constructed using a class Complex is serialized properly now. Not just for Oracle, but everywhere.

// FIXME: PHP 7.4 handles object serialization differently so we store 'Object' here
// to keep old (wrong) behavior and fix Oracle failure

this means: All DBMS were storing Object instead of the correct value. In addition Oracle was exploding, that's why I added that fixme - to store Object for all DBMS.
After this PR is merged there is no need in the fixup - the value is stored and restored properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants