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

Allow DAV Object properties #30368

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Allow DAV Object properties #30368

merged 1 commit into from
Jun 10, 2022

Conversation

tcitworld
Copy link
Member

@tcitworld tcitworld commented Dec 21, 2021

The current implementation only saves them as string. It seems they can be more complex than that, and that objects were saved directly.

You may find such objects saved in some production databases by executing:

SELECT * from oc_properties where propertyvalue = 'Object';

This seems to cause issues in some particular cases (nextcloud/calendar#3551) like the calendar trashbin because of things like this and (apparently ?) Apple devices:

$calendar['{DAV:}resourcetype'] = new DAV\Xml\Property\ResourceType([
'{DAV:}collection',
sprintf('{%s}deleted-calendar', \OCA\DAV\DAV\Sharing\Plugin::NS_NEXTCLOUD),
]);

This commit adds a repair job to clean all of these "broken" properties values, adds a new database column to save the type of the property, and handles converting from and to correct values.

Implementation is very similar to SabreDAV's own PDO backend: https://github.com/nextcloud/3rdparty/blob/4921806dfb1c5c309eac60195ed34e2749baf3c1/sabre/dav/lib/DAV/PropertyStorage/Backend/PDO.php

Fixes nextcloud/calendar#3858 (comment)

@tcitworld tcitworld added bug 3. to review Waiting for reviews feature: dav php Pull requests that update Php code feature: caldav Related to CalDAV internals labels Dec 21, 2021
@tcitworld tcitworld added this to the Nextcloud 24 milestone Dec 21, 2021
public function run(IOutput $output) {
$query = $this->connection->getQueryBuilder();
$updated = $query->delete('properties')
->where($query->expr()->eq('propertyname', $query->createNamedParameter(self::RESOURCE_TYPE_PROPERTY)))
Copy link
Member Author

Choose a reason for hiding this comment

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

Important note : I chose to delete only {DAV:}resourcetype properties because they were the ones involved in the calendar issue, but there might be more concerned propertynames.

I didn't want to directly remove the condition on propertyname in case there's some legitimate Object propertyvalues, (which seems however unlikely).

Copy link
Member Author

Choose a reason for hiding this comment

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

Found two extra property names affected:

  • {http://calendarserver.org/ns/}me-card
  • {urn:ietf:params:xml:ns:caldav}schedule-calendar-transp

@tcitworld
Copy link
Member Author

I think this is a better way to handle things than converting to string #30771

@tcitworld tcitworld requested review from a team and blizzz and removed request for a team February 15, 2022 09:25
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
This was referenced Apr 7, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@tcitworld tcitworld force-pushed the dav-allow-object-properties branch from 82a14e6 to 175fb56 Compare May 12, 2022 15:54
The current implementation only saves them as string. It seems they can
be more complex than that, and that objects were saved directly.

You may find such objects saved in some production databases by
executing:
```sql
SELECT * from oc_properties where propertyvalue = 'Object';
```

This commit adds a repair job to clean all of these "broken" properties
values, adds a new database column to save the type of the property, and
handles converting from and to correct values.

Implementation is very similar to SabreDAV's own PDO backend: https://github.com/nextcloud/3rdparty/blob/4921806dfb1c5c309eac60195ed34e2749baf3c1/sabre/dav/lib/DAV/PropertyStorage/Backend/PDO.php

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld tcitworld force-pushed the dav-allow-object-properties branch from 175fb56 to bd8b213 Compare May 16, 2022 13:02
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Makes sense, old known issue 👍

Also will likely solve issues with Oracle when storing complex props as I rembember issues there

@PVince81 PVince81 merged commit 2ee948e into master Jun 10, 2022
@PVince81 PVince81 deleted the dav-allow-object-properties branch June 10, 2022 15:07
@PVince81
Copy link
Member

25 only as it adds a new column and might be an expensive upgrade

tcitworld added a commit that referenced this pull request Jun 30, 2023
The propertyvalue column can contain null 0x00 characters values because of serializing PHP objects since #30368. This truncates data in text fields, but not blob fields. We start by removing invalid value and altering the column to match the new type.

That's what Sabre PDO's being doing in the first place 🙈

Closes #37754

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit that referenced this pull request Jul 2, 2023
The propertyvalue column can contain null 0x00 characters values because of serializing PHP objects since #30368. This truncates data in text fields, but not blob fields. We start by removing invalid value and altering the column to match the new type.

That's what Sabre PDO's being doing in the first place 🙈

Closes #37754

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit that referenced this pull request Jul 2, 2023
The propertyvalue column can contain null 0x00 characters values because of serializing PHP objects since #30368. This truncates data in text fields, but not blob fields. We start by removing invalid value and altering the column to match the new type.

That's what Sabre PDO's being doing in the first place 🙈

Closes #37754

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit that referenced this pull request Oct 15, 2024
The propertyvalue column can contain null 0x00 characters values because of serializing PHP objects since #30368. This truncates data in text fields, but not blob fields. We start by removing invalid value and altering the column to match the new type.

That's what Sabre PDO's being doing in the first place 🙈

Closes #37754

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit that referenced this pull request Oct 15, 2024
The propertyvalue column can contain null 0x00 characters values because of serializing PHP objects since #30368. This truncates data in text fields, but not blob fields. We start by removing invalid value and altering the column to match the new type.

That's what Sabre PDO's being doing in the first place 🙈

Closes #37754

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
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 bug feature: caldav Related to CalDAV internals feature: dav php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calendar update (to 3.0.4) resulted in endless loading of calendars
4 participants