Re: [EXTERNAL] Httpdir early review of draft-ietf-scim-cursor-pagination-00

On 09.05.2023 21:24, Danny Zollner wrote:
> Thank you for the review, Julian. We've just published a new version of the draft (-01) that should address most/all of your feedback. I'm adding notes in-line (prefixed with [DZ] ) to cover what changes were made in response to your feedback.
> 
> 
> -----Original Message-----
> From: Julian Reschke via Datatracker <noreply@ietf.org>
> Sent: Wednesday, March 22, 2023 4:24 AM
> To: ietf-http-wg@w3.org
> Cc: draft-ietf-scim-cursor-pagination.all@ietf.org; scim@ietf.org
> Subject: [EXTERNAL] Httpdir early review of draft-ietf-scim-cursor-pagination-00
> 
> [You don't often get email from noreply@ietf.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Reviewer: Julian Reschke
> Review result: Ready with Issues
> 
> # Questions
> 
> ## Introduction
> 
> "The two common patterns for result pagination in HTTP-based protocols ..."
> 
> Is this specific to HTTP based protocols?
> 
> [DZ] Changed to drop the HTTP-based protocols piece, now says "The two common patterns for result pagination are..."

Ok.

> ## Section 2
> 
> Maybe "Query parameter" should be defined (it's not part of HTTP). For instance, what happens if a cursor name obtained from a JSON response contains characters not allowed in a query parameter?
> 
> "Service Providers implementing cursor pagination that are unable to estimate totalResults MAY return a response with totalResults set to zero (0)."
> 
> Wouldn't it be clearer not to set totalResults at all?
> 
> [DZ] The term 'query parameters' is introduced in RFC 7644 3.4.2 and values are introduced throughout the 3.4.2.x section of the spec as filtering, sorting and pagination are defined. Given that, defining query parameters again doesn't seem necessary. If you disagree, happy to reconsider. For the totalResults piece, we've updated the guidance to: " Service Providers implementing cursor pagination that are unable to estimate totalResults MAY choose to omit the totalResults attribute."

Ok, this seems to be an issue with the base spec then. Anyway, what do 
you do with a cursor name obtained from JSON that can not be a query 
parameter name?


> ## Section 2.1
> 
> "For example, cursor pagination implementations SHOULD anticipate the following error conditions:"
> 
> Which follows seems to be a list of error conditions in prose. It's a bit unclear what the normative requirement is.
> 
> [DZ] Updated this section to be normative - explicitly states that it is extending the scimType values used in RFC 7644 3.12 to define error response types.

Ok.

> ## Section 2.3
> 
> What would be the HTTP status here?
> 
> [DZ] Fixed this - 200/OK.

Ok.

> ## Section 3
> 
> I'm confused by the term '"/.search" path extension'. The example URL "/User.search" does not seem to use that pattern.
> 
> [DZ] There was a typo here - example now states POST /User/.search which should be correct.

Ok.

> # Editorial
> 
> - please expand "SCIM" on first use
> 
> [DZ] Done.
> 
> Tables 1 and 2 seem to be good candidates for definition lists.
> 
> "A cursor value string that MAY be used in a subsequent request to obtain the previous page of results. Use of previousCursor is OPTIONAL. Service Providers that are unable to support a previousCursor MAY omit previousCursor when sending paged query responses."
> 
> The last sentence appears to be redundant.
> 
> [DZ] Agreed, fixed.

Ok.

> "The SCIM client MUST consider cursor to be opaque and make no assumptions about cursor values."
> 
> Maybe "Cursor values are opaque; clients MUST not make assumptions about their structure."
> 
> [DZ] Took your wording here and updated.

Ok.

> "The response to the query above returns metadata regarding pagination similar to the following example (actual resources removed for brevity)"
> 
> The example only shows the response content; either use the full message or rephrase.
> 
> [DZ] Expanded to full example.

Thanks.

> The last example could be read as showing a GET request with content; but the content is from the response, right?
> 
> [DZ] Adjusted structure to clarify.

Ok.

> ## Section 3
> 
> "Section 3.4.2.4 of [RFC7644] defines how clients MAY execute the HTTP POST verb"
> 
> It's 3.4.3. Also, the correct term is "method", not "verb".
> 
> [DZ] Fixed both.

Ok.

> # Nits
> 
> - for some reason, the draft name does not appear on the front page of the HTML version (bug in xml2rfc?)
> 
> - Typo in "Rsesources"
> 
> - "When using "/.search", the client would pass the parameters defined in Section 2" - sentence incomplete?
> 
> - Example in Section 3 misses empty line before content.
> 
> - Definition list in Section 4 has weird indentation.
> 
> [DZ] For the nits - they should all be fixed. I may have introduced new formatting (probably indentation) issues however as between last version and this version the draft was rewritten into Markdown/Kramdown from its original XML format.

Thanks!

-- 
<green/>bytes GmbH, Hafenweg 16, D-48155 Münster, Germany
Amtsgericht Münster: HRB5782

Received on Wednesday, 10 May 2023 15:10:08 UTC