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

cache webcal calendars on server #10059

Merged
merged 2 commits into from
Nov 8, 2018

Conversation

georgehrke
Copy link
Member

@georgehrke georgehrke commented Jun 29, 2018

fixes #1497

ToDos:

  • adapt refresh cycle to refresh rate of subscription
    • this could actually be tricky. the interval set in the constructor is called before run, but we only know the corresponding subscription when we know the arguments for run
    • we can obviously keep track of the last actual update ourself and just run the job once an hour, but I'd prefer if IJobList could just handle that.
  • update subscription in database when remote returns 301
  • unit tests

@georgehrke georgehrke added the 2. developing Work in progress label Jun 29, 2018
@georgehrke georgehrke force-pushed the feature/1497/cache_webcal_calendars branch 4 times, most recently from 9d83b9f to 2cba151 Compare June 30, 2018 12:38
@georgehrke georgehrke added this to the Nextcloud 14 milestone Jun 30, 2018
@georgehrke georgehrke force-pushed the feature/1497/cache_webcal_calendars branch 3 times, most recently from 88c8f76 to 864e40f Compare June 30, 2018 20:58
@rullzer
Copy link
Member

rullzer commented Jul 2, 2018

@georgehrke I'd say for now just write a small abstraction class yourself to solve the extra check issue.
Then if it turns out to work corectly etc we can always merge it back to the server part

@MorrisJobke
Copy link
Member

Most likely nothing for 14 -> moving to 15.

@georgehrke georgehrke force-pushed the feature/1497/cache_webcal_calendars branch 4 times, most recently from b072922 to c0868f8 Compare October 26, 2018 12:25
@georgehrke georgehrke force-pushed the feature/1497/cache_webcal_calendars branch 4 times, most recently from fb7c1f4 to 1b17e2f Compare October 31, 2018 11:28
if ($calendarObjectsTable->hasIndex('calobjects_index')) {
$calendarObjectsTable->dropIndex('calobjects_index');
}
$calendarObjectsTable->addUniqueIndex(['calendarid', 'calendartype', 'uri'], 'calobjects_index');
Copy link
Member Author

Choose a reason for hiding this comment

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

@nickvergessen Will building new indices be an issue? Should it be outsourced to a migration step?

Copy link
Member

Choose a reason for hiding this comment

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

I think for this it is fine. Should be relatively fast.

However, a step first to see if there are duplicated might be needed.
As else it will just explode

Copy link
Member Author

Choose a reason for hiding this comment

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

It removes the old index consisting of calendarid, uri and creates a new index calendarid, calendartype, uri. And calendartype is the all new column that was introduced by the same migration and set to 0 for all existing entries. So there should be no duplicates.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. :) Carry on then

Copy link
Member Author

Choose a reason for hiding this comment

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

@rullzer I was generally not sure what's the cleanest / best approach here.
We have calendars and subscriptions in different tables: calendars respectively calendarsubscriptions. This was no issue so far since we didn't store objects related to subscriptions.

So basically there are two options

  • Have duplicate tables of calendarchanges, calendarobjects and calendarobjects_props for subscriptions
  • Introduce calendartype that indicates whether calendarid refers to the calendars or the subscriptions table.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with both ways - even if the second one would be cleaner you need to update all the existing queries, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, i guess the big question here is whether introducing the new column calendartype is reducing performance of day-to-day queries.

Copy link
Member

Choose a reason for hiding this comment

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

When it is used in queries just add an index to it and it should be fine.

@georgehrke
Copy link
Member Author

@tcitworld Can you please test? Are there any other remarks? :)

@georgehrke georgehrke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 31, 2018
tcitworld
tcitworld previously approved these changes Oct 31, 2018
@georgehrke
Copy link
Member Author

georgehrke commented Nov 7, 2018

The acceptance test results are unrelated.

The phan thing seems to be a faulty php-doc in Sabre/Dav.
https://github.com/nextcloud/3rdparty/blob/214c4155f587f5178d792fe4a839044bdc9982f1/sabre/vobject/lib/Splitter/ICalendar.php#L87

@georgehrke
Copy link
Member Author

Should I just use @phan-suppress-next-line?

@georgehrke
Copy link
Member Author

Just wanted to send a pull-request, but it's already fixed upstream: https://github.com/georgehrke/vobject/blob/master/lib/Splitter/ICalendar.php#L86

@blizzz
Copy link
Member

blizzz commented Nov 7, 2018

@georgehrke is the upstream released and reasonable to get this in? Otherwise, let's suppress it with a note/issue to remove the suppression once we pull again.

@georgehrke
Copy link
Member Author

The fix was not released yet, adding @phan-suppress-next-line ...

@georgehrke
Copy link
Member Author

@MorrisJobke How to update phan?
We are running phan 0.11.1, but I need 0.12.9 to be able to use @phan-suppress-next-line

@blizzz
Copy link
Member

blizzz commented Nov 7, 2018

@georgehrke georgehrke force-pushed the feature/1497/cache_webcal_calendars branch from 31ac4b6 to cc66f65 Compare November 7, 2018 11:06
/**
* @param $subscriptionId
*/
public function getSubscriptionById($subscriptionId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When int $subscriptionId works for bigint colums i would use it here.

@MorrisJobke MorrisJobke mentioned this pull request Nov 7, 2018
29 tasks
@georgehrke georgehrke force-pushed the feature/1497/cache_webcal_calendars branch 3 times, most recently from 730c48e to 8967e85 Compare November 7, 2018 12:08
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@MorrisJobke
Copy link
Member

Please update the 3rdparty repo due to
nextcloud/3rdparty#185

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke georgehrke force-pushed the feature/1497/cache_webcal_calendars branch from 8967e85 to 712b79e Compare November 7, 2018 15:11
@georgehrke
Copy link
Member Author

Failing tests seem unrelated to me

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.

Code looks good to me. Lets do this.

@MorrisJobke MorrisJobke merged commit 0fb74af into master Nov 8, 2018
@MorrisJobke MorrisJobke deleted the feature/1497/cache_webcal_calendars branch November 8, 2018 09:55
@ChristophWurst
Copy link
Member

FYI: This caused a regression: #12410

@BernieO
Copy link
Contributor

BernieO commented Aug 22, 2019

I accidentally stumpled accross this when debugging an abnormality with my script calcardbackup and noticed that this is not in the changelog for Nextcloud 15

This commit adds events with the same calendarid belonging to different calendars ("regular" calendars and "subscribed" calendars). To determine to which calender the events belong there is a new database field calendartype added.

All this are quite big changes which are definitely worth to be mentioned in the changelog. Could this new featur please be added to the changelog?

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 enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose subscribed webcal calendars via CalDAV
9 participants