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

Handle long dav property paths by hashing them #19242

Merged
merged 8 commits into from
Mar 18, 2020
Merged

Conversation

icewind1991
Copy link
Member

Alternative to extending the column (#18126).

Even ignoring the migration problems with extending the problem, we then run into issues where we can't have an index on long string columns, which would require adding a separate path_hash column to query on.

Since we don't actually ever query the path from the table and only use it for searching, simply storing only the hash works just fine.

PR includes

  • Getting rid of a nearly duplicate version of the custom properties backend
  • Make the tests run against a real DB, not just expect specific queries
  • Extend the tests to cover more code paths
  • Hash paths when they are to long to store

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Jan 31, 2020
@icewind1991 icewind1991 added this to the Nextcloud 19 milestone Jan 31, 2020
Copy link
Member

@georgehrke georgehrke left a comment

Choose a reason for hiding this comment

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

This breaks custom properties for CalDAV (and probably CardDAV as well) for me:

To reproduce, just open the calendar app and click a calendar to hide it. When reloading, it should still be hidden, but it's not.

@georgehrke
Copy link
Member

Neither Calendar nor AddressBook are extending Node

@icewind1991
Copy link
Member Author

icewind1991 commented Feb 3, 2020

switched to INode

@icewind1991
Copy link
Member Author

nevermind INode doesn't have getPath switched back to just using the path

@icewind1991
Copy link
Member Author

cc @georgehrke @rullzer

Signed-off-by: Robin Appelman <robin@icewind.nl>
test behaviour not implementation

Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
@rullzer rullzer force-pushed the dav-long-properties branch 2 times, most recently from 5a40940 to e44c276 Compare March 18, 2020 12:49
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Lets do this!

@rullzer rullzer merged commit b306a81 into master Mar 18, 2020
@rullzer rullzer deleted the dav-long-properties branch March 18, 2020 18:48
@icewind1991
Copy link
Member Author

/backport to stable18

@icewind1991
Copy link
Member Author

/backport to stable17

@backportbot-nextcloud
Copy link

The backport to stable17 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

backport to stable18 in #20030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants