Fix davx5 missing e-tag error. #9

aharter · 2020-12-22T23:20:04Z

This PR should fix #3, #6 and #8.

I haven't checked the RFC, so I'm not sure if this is according to standard. Especially the filter functions. Which is why I opened it as a draft. Let's discuss.

HeikoTheissen · 2020-12-23T09:50:07Z

This fixes the two problems I reported in my comments.

src/common/xBuild.ts

example/data.js

sedenardi · 2020-12-29T15:23:58Z

example/data.js

+    return false;
+  }
+
+  const eventStart = event.startDate


Please match the coding style of the repository and include semicolons at the end of statements.

Seems like eslint is not linting the example files. I can add a commit or create a new PR to solve this. What do you think?

sedenardi · 2020-12-29T15:25:15Z

Thanks for the PR, sorry for the delay in reviewing. A couple things:

Can you post the request and response bodies before and after this change so we can ensure they're still conforming to the RFC? I don't have an Android device to test it with so I have to rely on yall to show what is being requested and returned.
There are a few coding style changes that need to be made to make the PR code consistent with the rest of the repository.

Once those things are addressed, I'll review the changes and the examples provided, as well as test the changes to make sure they still work with the iOS and MacOS CalDAV implementations (which was the original intent of this library).

To link other issues (only #3 was linked in the first PR comment):

Fixes #6
Fixes #8

aharter · 2021-01-02T18:33:43Z

I made the requested style changes.

I also changed the eventFilter logic from (!end || end < eventStart) || (!start || start > eventEnd) to (!start || start <= eventEnd) && (!end || end >= eventStart) after re-reading the RFC.

Here is the before and after comparison of request/response:
REQUEST PROPFIND /caldav/cal/user@ex.co/exampleCal1/

<?xml version='1.0' encoding='UTF-8' ?>
<CAL:calendar-query xmlns="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav">
  <prop>
    <getetag />
  </prop>
  <CAL:filter>
    <CAL:comp-filter name="VCALENDAR">
      <CAL:comp-filter name="VEVENT">
        <CAL:time-range start="20201004T154527Z" />
      </CAL:comp-filter>
    </CAL:comp-filter>
  </CAL:filter>
</CAL:calendar-query>

RESPONSE BEFORE

<?xml version="1.0"?>
<D:multistatus xmlns:D="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CS="http://calendarserver.org/ns/" xmlns:ICAL="http://apple.com/ns/ical/">
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:supported-report-set>
          <D:supported-report>
            <D:report>
              <CAL:calendar-query/>
            </D:report>
          </D:supported-report>
          <D:supported-report>
            <D:report>
              <CAL:calendar-multiget/>
            </D:report>
          </D:supported-report>
          <D:supported-report>
            <D:report>
              <CAL:sync-collection/>
            </D:report>
          </D:supported-report>
        </D:supported-report-set>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/41c6fcc0-aea7-304f-983a-a6fb7e568109.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/faf52014-372e-44ed-9eba-2fc1a2b8d82d.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/895a04b2-30df-4927-ba03-dfc55514f6e7.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/30946424-40f4-47e2-80b1-006fdebbeb25.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
</D:multistatus>

RESPONSE AFTER

<?xml version="1.0"?>
<D:multistatus xmlns:D="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CS="http://calendarserver.org/ns/" xmlns:ICAL="http://apple.com/ns/ical/">
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/41c6fcc0-aea7-304f-983a-a6fb7e568109.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getetag>20201122T100456Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/faf52014-372e-44ed-9eba-2fc1a2b8d82d.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getetag>20201122T100456Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/895a04b2-30df-4927-ba03-dfc55514f6e7.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getetag>20201122T100456Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/30946424-40f4-47e2-80b1-006fdebbeb25.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getetag>20201122T100456Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
</D:multistatus>

Note
The missing parts in the before response (with <D:supported-report-set>) was generated by the previous request

sedenardi · 2021-01-15T02:46:43Z

Apologies for the delay in reviewing this, I'll take some time in the next few days to take a look and give my feedback.

sedenardi · 2021-02-05T05:15:10Z

@aharter When I use curl to send the request in your example, I can't reproduce your findings. The responses on master and your branch are identical.

curl request

curl \
  --request PROPFIND \
  -H "Content-Type: text/xml" \
  --compressed \
  --url "http://localhost/caldav/cal/user@ex.co/exampleCal1/" \
  -u "user@ex.co:pass" \
  --data-binary @- << EOF
<?xml version='1.0' encoding='UTF-8' ?>
<CAL:calendar-query xmlns="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav">
  <prop>
    <getetag />
  </prop>
  <CAL:filter>
    <CAL:comp-filter name="VCALENDAR">
      <CAL:comp-filter name="VEVENT">
        <CAL:time-range start="20201004T154527Z" />
      </CAL:comp-filter>
    </CAL:comp-filter>
  </CAL:filter>
</CAL:calendar-query>
EOF

Response

<?xml version="1.0"?>
<D:multistatus xmlns:D="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CS="http://calendarserver.org/ns/" xmlns:ICAL="http://apple.com/ns/ical/">
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 404 Not Found</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/41c6fcc0-aea7-304f-983a-a6fb7e568109.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/faf52014-372e-44ed-9eba-2fc1a2b8d82d.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/895a04b2-30df-4927-ba03-dfc55514f6e7.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
</D:multistatus>

A couple things I noticed in the request

<prop> should be <A:prop>, right? Did you write the request, or did it come from the app?
<getetag /> should be <A:getetag /> - same as above

but changing those doesn't seem to have any effect.

The getetag element isn't being parsed properly, because it's not in the element array when the XML body is parsed.

However, the etag is present in the response when I make this request:

curl request

curl \
  --request PROPFIND \
  -H "Content-Type: text/xml" \
  --compressed \
  --url "http://localhost:3001/caldav/cal/user@ex.co/exampleCal2/" \
  -u "user@ex.co:pass" \
  --data-binary @- << EOF
<?xml version="1.0" encoding="UTF-8"?>
<A:propfind xmlns:A="DAV:">
  <A:prop>
    <A:getcontenttype/>
    <A:getetag/>
  </A:prop>
</A:propfind>
EOF

response

<?xml version="1.0"?>
<D:multistatus xmlns:D="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CS="http://calendarserver.org/ns/" xmlns:ICAL="http://apple.com/ns/ical/">
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal2/</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getcontenttype>text/calendar; charset=utf-8; component=VEVENT</D:getcontenttype>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal2/b5292608-2548-0041-8d7a-d7d6257d0b58.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getcontenttype>text/calendar; charset=utf-8; component=VEVENT</D:getcontenttype>
        <D:getetag>20210205T043707Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal2/a3f14917-7607-d94b-bcd8-1d0a9cf408f8.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getcontenttype>text/calendar; charset=utf-8; component=VEVENT</D:getcontenttype>
        <D:getetag>20210205T043707Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal2/30946424-40f4-47e2-80b1-006fdebbeb25.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getcontenttype>text/calendar; charset=utf-8; component=VEVENT</D:getcontenttype>
        <D:getetag>20210205T043707Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
</D:multistatus>

So it's either I'm incorrectly implementing the RFC (by looking for prop elements only if they're nested in a propfind element), or whatever client you're using is sending improper requests.

To be clear, I largely wrote this library based on the requests and responses that the MacOS/iOS clients send, and used the RFC as as clarifying guide. I'll try to dig into the RFC later on and see whether I should be considering prop element requests when not nested inside of a propfind element request, but I encourage you to do so as well.

I also apologize again for the long delay. Work mixed with having to re-familiarize myself with the library, as I haven't done significant work on it since middle of 2019.

src/common/xBuild.ts

sedenardi · 2021-02-24T04:05:52Z

I've tested this PR with MacOS and iOS, and everything looks good. I made a small comment regarding code conventions, but other than that it looks ready to mark as "Ready for review". Once you've done that I'll do another pass and merge if accepted.

Thanks again for your help and patience.

aharter · 2021-02-26T21:28:38Z

Thanks for taking the time to review the PR. I resolved your comments, if you have further additions please let me know.
Next step for me would be figuring out, whether I port my application to koa or create some PR support expressjs. I guess for the later your thoughts around it would be crucial.

EDIT:
I also took the liberty to rebase my changes on top of the current master

src/common/xBuild.ts

During a request, response elements are added to the global nsMap. Theses changes are usually overwritten by every request. However, if a request has no results, this does not happen, causing the previouse responses to be returned. This is circumvented by generating the nsMap for every request.

sedenardi · 2021-03-02T00:23:49Z

@aharter @HeikoTheissen Thanks to you both for your contributions. I've just published a new version to npm at 4.2.0.

Next step for me would be figuring out, whether I port my application to koa or create some PR support expressjs. I guess for the later your thoughts around it would be crucial.

I originally wrote the library with intention to expand it with express middleware support. It wouldn't take too much work to generalize the route and method handler functions to take in an object with context/request fields (not exhaustive):

method
url
params (raw and parsed)
authenticated user

and I'd certainly support such an effort if you wanted to take it on.

aharter marked this pull request as draft December 22, 2020 23:20

sedenardi requested changes Dec 29, 2020

View reviewed changes

This was linked to issues Dec 29, 2020

getEventsByDate seems to have not the correct response for iCal #6

Closed

xBuild multistatus handles empty responses incorrectly #8

Closed

aharter force-pushed the android-etag branch from bd58318 to 24dd7b9 Compare January 2, 2021 18:17

aharter force-pushed the android-etag branch 2 times, most recently from 38461f0 to 1bdc9b6 Compare January 2, 2021 19:27

aharter requested a review from sedenardi January 2, 2021 19:29

aharter force-pushed the android-etag branch from 1bdc9b6 to 980193c Compare January 5, 2021 17:12

HeikoTheissen reviewed Feb 5, 2021

View reviewed changes

src/common/xBuild.ts Outdated Show resolved Hide resolved

sedenardi reviewed Feb 24, 2021

View reviewed changes

src/common/xBuild.ts Outdated Show resolved Hide resolved

aharter added 2 commits February 26, 2021 22:20

Handle 'start' or 'end' filter not being defined

fad6bd4

Fix path argument of getWithChildren not being used

647c4f3

aharter force-pushed the android-etag branch from 980193c to 798bb3d Compare February 26, 2021 21:23

aharter marked this pull request as ready for review February 26, 2021 21:24

aharter requested a review from sedenardi February 26, 2021 21:28

HeikoTheissen suggested changes Feb 27, 2021

View reviewed changes

src/common/xBuild.ts Outdated Show resolved Hide resolved

aharter added 2 commits February 28, 2021 09:36

Fix event filter function in example

402e91b

aharter force-pushed the android-etag branch from 798bb3d to 402e91b Compare February 28, 2021 08:51

HeikoTheissen approved these changes Feb 28, 2021

View reviewed changes

sedenardi approved these changes Mar 2, 2021

View reviewed changes

sedenardi merged commit 57ed4e8 into forwardemail:master Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix davx5 missing e-tag error. #9

Fix davx5 missing e-tag error. #9

aharter commented Dec 22, 2020

HeikoTheissen commented Dec 23, 2020

sedenardi Dec 29, 2020

aharter Jan 2, 2021

sedenardi commented Dec 29, 2020 •

edited

Loading

aharter commented Jan 2, 2021 •

edited

Loading

sedenardi commented Jan 15, 2021

sedenardi commented Feb 5, 2021

sedenardi commented Feb 24, 2021

aharter commented Feb 26, 2021 •

edited

Loading

sedenardi commented Mar 2, 2021

Fix davx5 missing e-tag error. #9

Fix davx5 missing e-tag error. #9

Conversation

aharter commented Dec 22, 2020

HeikoTheissen commented Dec 23, 2020

sedenardi Dec 29, 2020

Choose a reason for hiding this comment

aharter Jan 2, 2021

Choose a reason for hiding this comment

sedenardi commented Dec 29, 2020 • edited Loading

aharter commented Jan 2, 2021 • edited Loading

sedenardi commented Jan 15, 2021

sedenardi commented Feb 5, 2021

curl request

Response

curl request

response

sedenardi commented Feb 24, 2021

aharter commented Feb 26, 2021 • edited Loading

sedenardi commented Mar 2, 2021

sedenardi commented Dec 29, 2020 •

edited

Loading

aharter commented Jan 2, 2021 •

edited

Loading

aharter commented Feb 26, 2021 •

edited

Loading