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

GEO field can break Google Calendar import #618

Closed
Timebutt opened this issue Oct 10, 2024 · 9 comments
Closed

GEO field can break Google Calendar import #618

Timebutt opened this issue Oct 10, 2024 · 9 comments

Comments

@Timebutt
Copy link

Timebutt commented Oct 10, 2024

I got a report from some of my customers that my calendar integration wasn't working for some people when they subscribed to my iCal feed in Google Calendar. The iCal file I generate validated correctly on a few of the online tools out there, and there were no issues when subscribing in Apple Calendar or Outlook. I started investigating (which meant cavemanning, Google Calendar doesn't exactly throw a stacktrace when the import goes wrong), and found Google Calendar breaks on this line of the *.ics file ical-generator builds:

GEO:null;null

Below demo calendar file is what you can use to validate my statement. If you go to Google Calendar, go to Settings and then import this file, you'll be greeted with a dialog saying Google Calendar can't import the events from the file.

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//Test//iCal Sync//EN
NAME:Test
X-WR-CALNAME:Test
REFRESH-INTERVAL;VALUE=DURATION:PT6H
X-PUBLISHED-TTL:PT6H
BEGIN:VEVENT
UID:2MIfT3GAVqBxqxKI7RUz
SEQUENCE:0
DTSTAMP:20241009T074546Z
DTSTART;VALUE=DATE:20250317
X-MICROSOFT-CDO-ALLDAYEVENT:TRUE
X-MICROSOFT-MSNCALENDAR-ALLDAYEVENT:TRUE
SUMMARY:Event Title
GEO:null;null
END:VEVENT
END:VCALENDAR

I haven't read RFC2445 yet to verify if GEO:null;null is a valid field yet, but as the validators seem to accept it either they are too lax, or Google is too strict in their implementation. Be that as it may: in the real world this results in a broken ics file so I think we have to do something here.

Easiest fix would be to omit the field if it's clearly empty (you can verify this, removing the line and importing the file works fine in Google Calendar), but I'm sure as to what you think is the best way forward?

I will patch this on my end for now, as I have clients waiting for a fix, but I'm happy to help implement a solution to mitigate this!

Edit: I was able to fix this fairly easily on my end by checking if the values I pass into location.geo actually exist. You could argue it's the developer responsibility to not pass in non-existing values, then again the library should not be able to generate invalid ics files either.

@sebbo2002
Copy link
Owner

Hi, can you provide a minimalistic example that produces your GEO:null;null output? Otherwise, unfortunately, I can't really reproduce it.

No location

import ical from 'ical-generator';

const cal = ical();

cal.createEvent({
  summary: 'Test 1',
  start: new Date(),
  location: null
});

cal.toString();
BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//sebbo.net//ical-generator//EN
BEGIN:VEVENT
UID:0154906d-e7ff-4724-83b0-84815b0294ab
SEQUENCE:0
DTSTAMP:20241010T140503Z
DTSTART:20241010T140503Z
SUMMARY:Test 1
END:VEVENT
END:VCALENDAR

With location.geo = null (invalid)

import ical from 'ical-generator';

const cal = ical();

cal.createEvent({
  summary: 'Test 1',
  start: new Date(),
  location: {
    geo: null
  }
});

cal.toString();
Error: `location` isn't formatted correctly. See https://sebbo2002.github.io/ical-generator/develop/reference/classes/ICalEvent.html#location

With location.geo.lat|lng = null (invalid)

import ical from 'ical-generator';

const cal = ical();

cal.createEvent({
  summary: 'Test 1',
  start: new Date(),
  location: {
    geo: {
      lat: null,
      lng: null
    }
  }
});

cal.toString();
Error: `location` isn't formatted correctly. See https://sebbo2002.github.io/ical-generator/develop/reference/classes/ICalEvent.html#location

@Timebutt
Copy link
Author

Timebutt commented Oct 10, 2024

I currently don't have the codebase on hand, but if I'm not mistaken this was what I was passing in:

{
   "location":{
      "geo":{
         "lat":"undefined",
         "lng":"undefined"
      }
   }
}

I can take a look tonight in the actual implementation at what it actually was if this doesn't turn out to reproduce the issue

@sebbo2002
Copy link
Owner

This also produces an error for me. So please have a look tonight or in the next few days. If it is a valid usage according to the API description, the output should of course also be valid and not null;null.

import ical from 'ical-generator';

const cal = ical();

cal.createEvent({
  summary: 'Test Event',
  start: new Date(),
  location: {
    geo: {
      lat: "undefined",
      lng: "undefined"
    }
  }
});

cal.toString();
Error: `location` isn't formatted correctly. See https://sebbo2002.github.io/ical-generator/develop/reference/classes/ICalEvent.html#location

@Timebutt
Copy link
Author

Just took a quick look at my exact code, because indeed your example throws the same error when I run it.
The exact object I pass in for location was this:

"location":{
   "title":"Empty Venue",
   "address":null,
   "geo":{
      "lat":null,
      "lon":null
   }
}

The documentation seems to mention there are two types for location, one with and one without title. Extrapolating that makes me think there are two code paths, and the one with title is the one happily accepting the null for both lat and lon?

@sebbo2002
Copy link
Owner

Okay, let's go through that one by one:

  • As you rightly say, you use the interface ICalLocationWithTitle for the title
  • address is defined here as string | undefined. null is therefore not a valid value
  • Same applies for geo. It can either be a ICalGeo object (with lat and lon number values!) or undefined. Filling the ICalGeo object with null values is not defined.
  • Your problem occurs because the validation does not check for null, as this value is not allowed (isFinite(null) === true). From this point of view, it is not a bug. Of course, I would still adjust this in the medium term if it has already been noticed.
  • I would recommend using more typesafe environments such as Typescript in the future to avoid such errors as far as possible.

@sebbo2002 sebbo2002 self-assigned this Oct 11, 2024
@Timebutt
Copy link
Author

Interesting! Don't worry, I only write code in TypeScript but unfortunately in this codebase I don't have strict: true which would have definitely caught this issue. That being said, lots of codebases out there are not running in strict mode, or are using plain JavaScript. I think the only question is: is GEO:null;null a valid value according to the official specification. If it isn't, I feel the library should never be able to generate this and should be more robust in terms of what people can pass into the createEvent() function.

Now to make my codebase strict ;)

@sebbo2002 sebbo2002 mentioned this issue Oct 15, 2024
github-actions bot pushed a commit that referenced this issue Oct 15, 2024
## [8.0.1-develop.8](v8.0.1-develop.7...v8.0.1-develop.8) (2024-10-15)

### Bug Fixes

* **Event:** Handle location.geo.lat|lon = null ([7e68f00](7e68f00)), closes [#618](#618)
@sebbo2002
Copy link
Owner

🎉 This issue has been resolved in version 8.0.1-develop.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Timebutt
Copy link
Author

Thanks for the fix! I'm hoping this prevents other people from debugging huge iCal files that don't work in Google Calendar ;)

github-actions bot pushed a commit that referenced this issue Oct 16, 2024
## [8.0.1](v8.0.0...v8.0.1) (2024-10-16)

### Bug Fixes

* **Event:** Add missing : for ORGANIZERs ([dca6352](dca6352)), closes [#610](#610)
* **Event:** Handle location.geo.lat|lon = null ([7e68f00](7e68f00)), closes [#618](#618)
@sebbo2002
Copy link
Owner

🎉 This issue has been resolved in version 8.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

No branches or pull requests

2 participants