Date and Time Scalars #256

hellomika · 2017-01-28T03:22:39Z

I added 4 built-in scalars:

utc_datetime parses to a DateTime struct and serializes to an ISO8601 date and time + UTC timezone marker string
datetime parses to a NativeDateTime struct and serializes to an ISO8601 date and time string
date parses to a Date struct and serializes to an ISO8601 date string
time parses to a Time struct and serializes to an ISO8601 time string

aweiker · 2017-01-28T06:22:38Z

lib/absinthe/type/built_ins/scalars.ex

@@ -59,6 +59,48 @@ defmodule Absinthe.Type.BuiltIns.Scalars do
    parse parse_with([Absinthe.Blueprint.Input.Boolean], &parse_boolean/1)
  end

+  scalar :utc_datetime, name: "DateTime" do


Was there a specific reason to need to opt-in to timezone? I know for me, I prefer to default to being overly specific which would be using a timezone by default. This also goes against the assumptions of the Elixir conventions of DateTime is specific while NaiveDateTime is not specific. Is there any reason not to use :naive_datetime as the identifier to keep that consistency?

I was following the same conventions than Ecto (which I concede might be driven by legacy). I was also considering what an API consumer would see in introspection and I thought this choice would make the most sense. That said being more specific doesn't hurt :)

Yeah given that we're using the native Elixir date time handling I think we should go with :datetime as always UTC.

bruce · 2017-01-28T07:38:56Z

I'm open to the idea of packaging additional, useful types (provided they only use the standard library), but I'm not keen on the idea of including types that go beyond the GraphQL Specification as built-ins -- both because of the possibility of future collision with "official" types and because we already have users with types that match these names but may have different accepted date formats, etc.

I think the reasonable choices here come down to one of:

We launch a separate package (eg, absinthe_contrib) that provides these types.
We package them along with Absinthe proper, but somewhere other than in Absinthe.Type.BuiltIns

In either case, use of the types by schema designers should probably be opt-in via import_types.

hellomika · 2017-01-28T11:46:48Z

@bruce that make sense, I didn't consider the standard scalars from the official specs. They even mention ISO-8601 time string as an example of custom scalar.

I like both ideas. They could be packaged in Absinthe.Utils.Scalar. Using one of them would look like import_types Absinthe.Utils.Scalar.NaiveDateTime.

About the build failing: it looks like the container installs an older version of Elixir lacking DateTime.from_iso8601/1.

benwilson512 · 2017-01-28T22:49:46Z

I think we make an Absinthe.Type.Extensions module that contains all the date time types we have here. We can already do import_types Absinthe.Type.Extensions, only: [:datetime] if someone wants just one type or another, so we don't need to give each type its own module.

hellomika · 2017-01-29T14:05:59Z

Update up for review. The build is still failing due to Elixir 1.3.0.

jparise · 2017-02-02T10:06:47Z

lib/absinthe/type/extensions.ex

+  @spec parse_datetime(any) :: {:ok, DateTime.t} | :error
+  defp parse_datetime(value) when is_binary(value) do
+    case DateTime.from_iso8601(value) do
+      {:ok, datetime, _offset} -> {:ok, datetime}


I have something similar in my code, but I also consider a non-zero offset to be an input error:

case DateTime.from_iso8601(string) do {:ok, dt, 0} -> {:ok, dt} {:ok, _, _} -> {:error, :unsupported_offset} {:error, reason} -> {:error, reason} end

That policy might be too strict for a general implementation, but I think it's slightly more honest about how offset information is treated by the server.

benwilson512 · 2017-02-05T21:58:51Z

Can you update the .travis.yml to Elixir 1.4?

benwilson512

This PR could use some integration level tests. Build a small schema try to do some queries with each of these types as both inputs and return values.

benwilson512 · 2017-02-06T02:24:00Z

lib/absinthe/type/extensions.ex

+    """
+
+    serialize &DateTime.to_iso8601/1
+    parse parse_with([Absinthe.Blueprint.Input.DateTime], &parse_datetime/1)


I think you're misunderstanding what parse_with does. These are all gonna be Input.String types because that's the Blueprint type that these datetime strings will initially come in as. They get parsed into datetimes sure, but prior to that they're just strings. The list you pass to parse_with is a list of the basic input types that are acceptable.

Got it! I should have better reviewed when I moved the code from builtins to extensions :)

I'll update soon.

jparise · 2017-02-07T17:24:35Z

This is a very minor comment, but I have a small preference for .Custom over .Extensions to keep the terminology in-line with the GraphQL spec's language. How does that sit with the rest of you?

tlvenn · 2017-03-05T01:49:33Z

@hellomika any update on this PR ? Thanks in advance.

benwilson512 · 2017-03-05T02:56:29Z

We are about to release a 1.3 beta, and it would be nice if we could include these in here. I may just merge them into the 1.3 branch and then make some edits tomorrow, we'll see.

hellomika · 2017-03-05T12:17:55Z

Hey all, apologies for the delay, time goes by fast :) I'm working on it right now.

jparise

I added a few notes about documenting and testing the non-zero-offset behavior. Other than that, this looks great, and I'll be happy to remove my local implementations in favor of these. Thanks!

jparise · 2017-03-06T01:46:30Z

.travis.yml

@@ -1,5 +1,5 @@
 language: elixir
-elixir: 1.3.0
+elixir: 1.4.1


May as well go to Elixir 1.4.2 now.

jparise · 2017-03-06T01:50:07Z

lib/absinthe/type/custom.ex

+    description """
+    The `DateTime` scalar type represents a date and time in the UTC
+    timezone. The DateTime appears in a JSON response as an ISO8601 formatted
+    string, including UTC timezone ("Z").


It might be helpful to include a brief sentence about the input format's expectations, too. Parsing an input string with a timezone offset other than 0 is considered an error.

jparise · 2017-03-06T01:52:34Z

test/lib/absinthe/type/custom_test.exs

+      assert {:ok, @datetime} == parse(:datetime, "2017-01-27 20:31:55Z")
+    end
+
+    it "cannot be parsed without UTC timezone marker" do


Please also include a test for parsing a non-0 offset (which should also be an error).

benwilson512 · 2017-03-06T21:27:07Z

lib/absinthe/type/custom.ex

+  end
+
+  @spec parse_datetime(any) :: {:ok, DateTime.t} | :error
+  defp parse_datetime(value) when is_binary(value) do


These functions are not passed simply a string IE "2016-01-01T12:12:12". Rather they are passed an input blueprint struct. You'll want to pattern match that a string input has been given by doing:

defp parse_datetime(%Absinthe.Blueprint.Input.String{value: value}) do

When you do so, there isn't really any need to do the is_binary check.

jparise · 2017-03-06T23:23:02Z

test/lib/absinthe/type/custom_test.exs

@@ -37,6 +38,11 @@ defmodule Absinthe.Type.CustomTest do
      assert {:ok, @datetime} == parse(:datetime, "2017-01-27 20:31:55Z")
    end

+    it "cannot be parsed when a UTC offset is included" do


Sorry to be pedantic, but I think should should be "non-zero UTC offset". I don't believe this string will not error: "2017-01-27T20:31:55+00:00"

Thanks, great feedback :)

hellomika · 2017-03-08T13:04:19Z

Changes up for review. Let me know if the high level tests are adequate!

benwilson512 · 2017-03-08T14:16:57Z

Merged into the 1.3 branch.

harmon25 · 2017-04-10T21:33:08Z

hey folks,
Looking at using the custom date scalar.
I guess 1.3 beta.0 does not have this feature implemented?
Getting the following error:

== Compilation error on file lib/longport/graphql/objects.ex ==
** (UndefinedFunctionError) function Absinthe.Type.Custom.__absinthe_types__/0 is undefined (module Absinthe.Type.Custom is not available)
    Absinthe.Type.Custom.__absinthe_types__()
    lib/absinthe/schema/notation.ex:1065: Absinthe.Schema.Notation.do_import_types/2

I will use the 1.3 branch as a mix dependency worst case...

Thanks!

benwilson512 · 2017-04-10T22:08:02Z

@harmon25 you should try the beta.2, I believe it's part of that release.

* Add logging, with support for filtering (#280) * Support log: false configuration * Address some feedback, clean up defaults (#282) * Middleware (#254) * minor optimization around arguments * basic functionality * almost green tests * green tests * handle middleware callbacks * move plugins over to the middleware namespace * forgot files as usual * significant progress * remove async and batch indirection * rename Absinthe.Resolution.Middleware => Absinthe.Middleware * handle before and after resolution * some middleware testing * improved resolution state names * plug is now middleware * finalize name for ensuring middleware * doc updates * it helps if you commit all the files * finalizing names * handle commentary * better doc header * Schema ensure query type (#226) * Add query type rule * Start to fix tests with querytype * Add test * fix more tests with query type * Remove double imports * Remove unused Type * Add Explanation to FieldImportExist * Fix the last two tests Rewinding my own mistakes * Fixed valid_schema * ensure anon function form works * context modification test * more docs * updated travis * last doc tweak * changelog update * version bump * correct version * typo fix * Date and Time Scalars (#256) * added utc_datetime as a built-in scalar * added naive datetime as a built-in scalar * added date as a built-in scalar * added time as a built-in scalar * removed datetime types from built_ins * moved datetime types to new extensions module * added stricter pattern matching to datetime parser * elixir 1.4 for travis * fixed parsing and renamed module * bumped elixir version * bumped elixir version for travis * testing against UTC offsets * testing parsing 0-utc offset * parsers are passed a blueprint input struct instead of a string * added high-level tests for custom types * build fix * Use Markdown list syntax for nicer rendering (#285) * changelog entry * doc fix * Support for `to_string/1` compatible error tuples in resolution (#286) * Use to_string for error messages Sometimes you get other types in {:error, value}, like atom and integers, and instead of people handling the converation themselves it's better just returning the string equivalent of the atom inline with what json libraries like Poison do or any other type that can be a string. An example would be if you are doing `with` in you body you get the error message from the library you are using. In guardian case you could get: `{:error, :token_expired}` * Changes for bruce * Add changelog entry * Moving TODOs to PR * better changelog for beta * fix bug where root query and mutation types had nil identifiers * version bump * provide test helping primer function * docs * Tuning (#292) * do expansion prior so that we don't have to do it in resolution * faster field applies check * get rid of infinite loops and improved type condition handling * add renamed file, and code reorganization * so close to green * tests are green at last * remove bug line * Fix #244 * Interface subtype (#293) * Add failing test for interface subtype (#288) This shows that an implementor of an interface with a more specific field than expected will not compile. * test fixes * fixes to enable absinthe relay * test prime bug fix * more assertive * resurrect plugin module as the before/after callbacks from middleware * middleware.t => middleware.spec * move scalar and enum serialization to the result phase (#296) * remove commented out debug statements * Add Absinthe.Pipeline.replace/3 (#300) * Add Pipeline.replace/3 * Better type spec for Type.Directive * Make Pipeline.replace/3 more flexible * Clean up some warnings * Prepare 1.3.0-beta.2 * Fix doc typo * Another doc fix * laxer test time for travis * Batch resolution phase (#302) * basic work on batching resolution phase * bug fix * remove warnings * Changed HandleAuth to HandleError (#297) * minor tweaks to support absinthe plug batching * Extensions (#306) * make the whole pipeline work with blueprints * extensions test * bug fixes * version bump * if a validation phase cannot jump, it returns an error instead of an ok tuple from the phase * remove strange indentation * fix performance issue, need to fix some tests though (#307) * fix performance issue, need to fix some tests though * green tests

aweiker reviewed Jan 28, 2017

View reviewed changes

bruce added Kind:Feature Status:WIP labels Jan 28, 2017

jparise reviewed Feb 2, 2017

View reviewed changes

benwilson512 requested changes Feb 6, 2017

View reviewed changes

hellomika added 8 commits March 5, 2017 07:13

added utc_datetime as a built-in scalar

9cfdca7

added naive datetime as a built-in scalar

25e45cd

added date as a built-in scalar

3235180

added time as a built-in scalar

8badd28

removed datetime types from built_ins

455b8f7

moved datetime types to new extensions module

d5bd6e1

added stricter pattern matching to datetime parser

a3052c4

elixir 1.4 for travis

afa213d

hellomika added 2 commits March 5, 2017 08:25

fixed parsing and renamed module

834668f

bumped elixir version

57d86ab

jparise approved these changes Mar 6, 2017

View reviewed changes

bumped elixir version for travis

41a5ffe

benwilson512 reviewed Mar 6, 2017

View reviewed changes

testing against UTC offsets

62a5a5f

jparise reviewed Mar 6, 2017

View reviewed changes

testing parsing 0-utc offset

faa0f3e

hellomika added 2 commits March 8, 2017 06:46

parsers are passed a blueprint input struct instead of a string

4cb6f00

added high-level tests for custom types

38a06c2

benwilson512 approved these changes Mar 8, 2017

View reviewed changes

jparise approved these changes Mar 8, 2017

View reviewed changes

benwilson512 changed the base branch from master to v1.3 March 8, 2017 14:16

benwilson512 merged commit a5ed765 into absinthe-graphql:v1.3 Mar 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Date and Time Scalars #256

Date and Time Scalars #256

hellomika commented Jan 28, 2017

aweiker Jan 28, 2017

hellomika Jan 28, 2017 •

edited

Loading

benwilson512 Jan 28, 2017

bruce commented Jan 28, 2017

hellomika commented Jan 28, 2017 •

edited

Loading

benwilson512 commented Jan 28, 2017

hellomika commented Jan 29, 2017

jparise Feb 2, 2017

benwilson512 commented Feb 5, 2017

benwilson512 left a comment

benwilson512 Feb 6, 2017

hellomika Feb 7, 2017

jparise commented Feb 7, 2017

tlvenn commented Mar 5, 2017

benwilson512 commented Mar 5, 2017

hellomika commented Mar 5, 2017 •

edited

Loading

jparise left a comment

jparise Mar 6, 2017

jparise Mar 6, 2017

jparise Mar 6, 2017

benwilson512 Mar 6, 2017

jparise Mar 6, 2017 •

edited

Loading

hellomika Mar 8, 2017

hellomika commented Mar 8, 2017

benwilson512 commented Mar 8, 2017

harmon25 commented Apr 10, 2017

benwilson512 commented Apr 10, 2017

Date and Time Scalars #256

Date and Time Scalars #256

Conversation

hellomika commented Jan 28, 2017

Choose a reason for hiding this comment

hellomika Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruce commented Jan 28, 2017

hellomika commented Jan 28, 2017 • edited Loading

benwilson512 commented Jan 28, 2017

hellomika commented Jan 29, 2017

Choose a reason for hiding this comment

benwilson512 commented Feb 5, 2017

benwilson512 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jparise commented Feb 7, 2017

tlvenn commented Mar 5, 2017

benwilson512 commented Mar 5, 2017

hellomika commented Mar 5, 2017 • edited Loading

jparise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jparise Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellomika commented Mar 8, 2017

benwilson512 commented Mar 8, 2017

harmon25 commented Apr 10, 2017

benwilson512 commented Apr 10, 2017

hellomika Jan 28, 2017 •

edited

Loading

hellomika commented Jan 28, 2017 •

edited

Loading

hellomika commented Mar 5, 2017 •

edited

Loading

jparise Mar 6, 2017 •

edited

Loading