Draft changes to support Dates and DateTime

[slodge]

This is a PR for #890 - providing Date and DateTime support

PR task list:

Functional:

• Discuss design and make sure the maintainer is happy...
• Discuss implementation and make sure the maintainer is happy...
• I suspect the regexes "could be better"...

Admin:

• Sign RStudio CLA
• Update NEWS
• Update articles
• Add tests
  • GET and POST query parameters
  • body parameters
  • dynamic route parameters
  • parameters which don't parse
• Run devtools::check()
• Update documentation with devtools::document()

Minimal reproducible example

Manual test files used so far:

#* Consider this...
#* @param when:date the date
#* @param t:date-time The date and time
#* @get /dates
function(when, t) {

  browser()
  paste0("The date was ", when, " and the time was ", t)
}

That seems to work:

Note: have also tested this alongside #889

[schloerke]

Looks good!

Requests:

• Can you add the date/datetime formats to annotations.Rmd's table in More details on `@param` ?
• Add a NEWS entry ending in (@slodge #891)

[schloerke]

For testing, please add tests where you see fit in tests/testthat/test-zzz-openapi.R

[schloerke]

This would allow for people to use

#* @param x:POSIXct Description of my date `x`

Since we are not doing anything in particular about the date type object, let's rearrange the values.

Suggested change

	c("POSIXct", "POSIXt"),
	c("datetime", "date-time"), # Keep `datetime` for convenience

[slodge]

I've got to admit I don't really follow what the current parameters for addApiInfo mean... and I think #889 might have changed what they mean...

As far as I can see, if we are heading towards auto-conversion by default, then in an apiInfo there should be:

• the type key for the entry - this will be what appears next to the @param name - e.g. @param my_field:int
• the locations this can be found (I guess... although I've not used this much)
• the openapi type that will be displayed (one of https://swagger.io/docs/specification/data-models/data-types/ ?)
• (optional) the format specifier to go alongside the openapi type
• (optional) the R parser that will be applied to parse the incoming character value (this could possibly also return errors if parsing unsuccessful?)
• (optional) the regex that is applied in the swagger ui
• (optional) possible other keywords like minimum, multipleOf, 'minLength`, etc (but think they live in a future PR 👍 )

Based on my understanding from this list... I don't really understand what apiType, plumberTypes and realType are

• if doing a major release, can we get rid of (or deprecate) some of these fields?

Would it help to clean up addApiInfo and replace it with a function with a surface something like:

addApiInfo(
    key, 
    locations = c("body", "route", "query"), 
    openApiType = c("string", "number", "integer", "boolean", "object"),
    # note that openApiFormat is extensible - so match.arg should not be used on openApiFormat
    openApiFormat = c("float", "double", "int32", "int64", "date", "date-time", "password", "byte", "binary"), 
    openApiRegex  = NULL,
    parser = function(input) { list(error=NULL, result=input) } 
 ) {
 ...
 }

That could then be called and configured with only one key for each entry (I think maybe plumber doesn't need this for loop any more -

plumber/R/openapi-types.R

Line 34 in befc866

for (plumberType in plumberTypes) {

)

Disclaimer: this is all coming very much from #889 and I admit I don't have any experience of the attachment/file and object inputs... they might cause more problems here?

Sorry... didn't mean to ask such "big questions"

[slodge]

I had a go at something like this on a different branch - see commits slodge@536dd29 and slodge@ae091eb

To keep backward portability, I kept all the synonyms - e.g. dbl and double

That change is almost complete now... there are a few more bits to do including the file/object stuff, and one test I'd like to look at a bit more... I wanted to send it in for discussion... Is this direction worth pursuing or is it too big a change for the core team?

It's not something I feel we desparately need - it is just a cleanup of api spec definitions to align them closer to the openApi types - rather than anything strictly functional...

[schloerke]

Much better variable names! Makes it much clearer as to what is happening. I'll admit it was fussy when I wrote it.

Nice work on the backwards compatibility.

Would be happy to merge the changes once complete.

[slodge]

Thanks.

Have got all tests passing (first half of Argentina vs Australia wasn't exciting so I had spare time!)

Have merged those changes into this PR.

Would really like to drop converterArray depending on decisions here and in the other PR - #889 (comment) - I guess we really only intend this ever to be used for [int] and in query paths...

I can merge the PRs into one big thing to review if that would make it easier?

[slodge]

Still open here:

• Add the date/datetime formats to annotations.Rmd's table in More details on @param (and more to discuss?)
• Add a NEWS entry ending in (@slodge Draft changes to support Dates and DateTime #891)
• Discuss/resolve this converterArray stuff

[slodge]

@schloerke apologies for this... but I think it might be sensible to merge this one after we work out what to do with #889

I don't think this has much use without the work in #889 ... and I think after committing #889 then there'll be other changes needed here (e.g. other tests for the converter)

Sorry - they're a bit inter-related...

[jcheng5]

I don't love the idea of the behavior being subtly different depending on whether this package is installed (we do this sometimes in Shiny and it always seems to end up hurting). Is there a semantic difference between the two?

If there is, and we strongly prefer lubridate's behavior, I'd prefer to throw an error if lubridate is not available.

[slodge]

From https://lubridate.tidyverse.org/reference/as_date.html

Compare to base R

These are drop in replacements for as.Date() and as.POSIXct(), with a few tweaks to make them work more intuitively.
Called on a POSIXct object, as_date() uses the tzone attribute of the object to return the same date as indicated by the printed representation of the object. This differs from as.Date, which ignores the attribute and uses only the tz argument to as.Date() ("UTC" by default).
Both functions provide a default origin argument for numeric vectors.
Both functions will generate NAs for invalid date format. Valid formats are those described by ISO8601 standard. A warning message will provide a count of the elements that were not converted.
as_datetime() defaults to using UTC.

That last point wins me over ... but should be possible to do in as.Date.POSIXct too?

I'd be very happy adding a permanent dependency to lubridate (would any plumber users really mind?), or switching to an error, or duplicating the lubridate code here (possibly with some cross-referencing to what open api spec says)

[jcheng5]

Is this intentionally limiting the timezone to Z?

[slodge]

I'm very happy to see it restricted... I encourage everyone to use UTC whenever they can (and if I can persuade the UK government to drop BST then it'd make my life so much easier...)

But from a RFC 3339 perspective, it could probably be extended.

As far as I understand it, this regex is only used in parsing path query variables (but I might have that wrong...). With that in mind, are these regexes needed at all?

If they are, then the comment points to stackoverflow.com/a/3143231/373321 with a wealth of options :)

[slodge]

Closing - too hard to maintain separate long-running PRs