Feature: Add Duration Scalar

[dmoree]

Description

Adds a Duration scalar. It is passed in as an ISO 8601 duration string and stored in the database as a neo4j duration value.

This allows for ISO 8601 duration strings PnYnMnDTnHnMnS, PnW, or PYYYYMMDDTHHMMSS.

Checklist

The following requirements should have been met (depending on the changes in the branch):

• Documentation has been updated
• TCK tests have been updated
• Integration tests have been updated
• Example applications have been updated
• New files have copyright header
• CLA (https://neo4j.com/developer/cla/) has been signed

[neo4j-team-graphql]

Thanks for the documentation updates.

The preview documentation has now been torn down - reopening this PR will republish it.

[dmoree]

@darrellwarde @danstarns There seems to be an issue with the neo4j-driver when specifying fractional values. Fractional values are allowed under the standard as long as they are on the smallest value (e.g. P2.5Y is valid, P2.5Y1M is not). When parsing a duration string, e.g. P66.5M, the Duration instance has the correct form, i.e.

{
  months: 66.5,
  days: 0,
  seconds: {
    low: 0,
    high: 0
  },
  nanoseconds: {
    low: 0,
    high: 0
  }
}

However, when this is stored in the database the fractional value is truncated and it stores the duration as P66M as opposed to P66.5M. I can look into this some more and possibly open an issue there if necessary, but I would like to know your initial thoughts.

Edit:
I found the function packDuration which appears to convert all elements into Integers before sending to the database. If this were not done for months or days then each of those values would be treated as a Float, which is how they should probably be treated.

[darrellwarde]

I found the function packDuration which appears to convert all elements into Integers before sending to the database. If this were not done for months or days then each of those values would be treated as a Float, which is how they should probably be treated.

Good find, for sure, it looks like that could be the culprit! Definitely raise an issue in the driver repo, however, it looks like responses can be a bit slow in there.

In the meantime, we have a couple of options!

• Leave this open until this is addressed in the driver
• Document that fractional durations cannot be used, update the RegEx to reflect this, and throw an informative error if someone tries to use fractional values

I would personally go for the latter - we're not forcing anyone to use it and are making the situation clear at the end of the day. Would take something like the following pseudocode for good error handing:

if (! match regex with NO fractional values) {
    if (match regex WITH fractional values) {
        throw new Error("You cannot use fractional values in duration values, please see documentation at WEB_ADDRESS");
    }
    throw new Error("Generic error, does not match duration RegEx");
}

[dmoree]

I decided to leave the regex the way it is and throw an error if a user tries to pass a duration string with decimal values as well as updating the documentation to reflect this. This way if the issue with the driver gets resolved then it will be a simple matter of removing this check.

I also added some test for validation and parsing.

[darrellwarde]

Damn, sorry, looks like another PR merge into master has broken the TCK tests, you'll just need to add CreateInfo and UpdateInfo types and a few fields into the duration.md schema test.

[darrellwarde]

I think in terms of getting features in for today's release, I'm going to call it a day before merging this in actually.

I'm just reading up on making comparisons between durations, which while isn't directly possible, can be achieved by adding the duration to a datetime value.

So the Cypher for a less than comparison could be for example:

MATCH (n:Node)
WHERE localtime() + $inputDuration < localtime() + n.duration
RETURN n

I think this type will have a lot more utility if we are able to add this additional functionality in, and get Duration working similarly to the other temporal scalar types.

Not a huge amount of research done, but background for this primarily in this old article: https://www.markhneedham.com/blog/2018/06/02/neo4j-3.4-comparing-durations/

EDIT: This also confirmed in docs, just above this "Resources" section: https://neo4j.com/developer/cypher/dates-datetimes-durations/#cypher-resources

[dmoree]

Understandable. In light of more work on comparisons that you noted and the bug with regards to floats, I also think it is better to come back to this. I appreciate all the work you and your team put in to make this library what it is. Thanks for the links.

[dmoree]

The regex has been refactored to have better group capture and hence readability when parsing. It also now captures the extended P<date>T<time> format. The comparison operators have been added according to the link above.

With this matching the full ISO 8601 duration and the comparison operations being in line with the other temporal fields, I will consider this complete for now and if/when the driver allows for floats the error for that case can be removed. Of course I could have always missed something and any changes or comments are welcome. Thanks.

EDIT:
I missed negative durations

[darrellwarde]

This looks great, a lot going on in that parseDuration function! 🤯

I think in terms of functionality, pretty happy, just a few comments about code formatting, and a couple of questions in parts. 🙂

Sorry for the delay on reviewing this one, a few absences in the team over the past week and a bit.

[darrellwarde]

value is always a string in parseValue, so you can stipulate that here and get rid of the first type check in parseDuration.

[dmoree]

It's JSON correct? It makes sense that this is a string, but I was relying of graphql which types this as any. I thought I was missing something.

Fixed in 273c26b

[darrellwarde]

Oh damn, looking back, maybe I'm the one who's missing something! 🙈 Honestly, I was basing it off the code in this project, for example the BigInt scalar: https://github.com/neo4j/graphql/blob/master/packages/graphql/src/schema/scalars/BigInt.ts#L30

But delving into it, perhaps it should be typed as any, my mistake. 😞 First day back after a week off yesterday, can I be excused? 😄

Definitely remove the type check as you have done in parseDuration, but you're right, it probably does need to be in here.

[dmoree]

No worries. Added back in 0d7f83f

[dmoree]

I apologize for parseDuration hopefully the comments help. I opened up an issue (neo4j/neo4j-javascript-driver#768) requesting the ability to pass floats as arguments to Duration. @bigmontz confirms that Neo4jBrowser uses that driver, but I seem limited with what I can pass to Duration. Ideally, I can pass it an object or string like I can in the browser and that would save some of the calculations here as they would move up to the driver level.

[darrellwarde]

I apologize for parseDuration hopefully the comments help. I opened up an issue (neo4j/neo4j-javascript-driver#768) requesting the ability to pass floats as arguments to Duration. @bigmontz confirms that Neo4jBrowser uses that driver, but I seem limited with what I can pass to Duration. Ideally, I can pass it an object or string like I can in the browser and that would save some of the calculations here as they would move up to the driver level.

I imagine you're code from parseDuration would do a lot of what is needed in the driver - you should consider adapting it and opening it as a PR there! 🙂

Definitely close on this one, apologies for my slight error in review yesterday.

[darrellwarde]

Hey @dmoree, we've just merged in a branch which refactors the schema TCK tests to use Jest Snapshots, which has obviously broken this branch! You're going to need to delete the packages/graphql/tests/tck/tck-test-files/schema directory, and create a new file packages/graphql/tests/schema/types/duration.test.ts with the following content:

/*
 * Copyright (c) "Neo4j"
 * Neo4j Sweden AB [http://neo4j.com]
 *
 * This file is part of Neo4j.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

import { printSchemaWithDirectives } from "@graphql-tools/utils";
import { lexicographicSortSchema } from "graphql/utilities";
import { gql } from "apollo-server";
import { Neo4jGraphQL } from "../../../src";

describe("Duration", () => {
    test("Duration", () => {
        const typeDefs = gql`
            type Movie {
                id: ID
                duration: Duration
            }
        `;
        const neoSchema = new Neo4jGraphQL({ typeDefs });
        const printedSchema = printSchemaWithDirectives(lexicographicSortSchema(neoSchema.schema));

        expect(printedSchema).toMatchInlineSnapshot(`
            "schema {
              query: Query
              mutation: Mutation
            }

            \\"\\"\\"A BigInt value up to 64 bits in size, which can be a number or a string if used inline, or a string only if used as a variable. Always returned as a string.\\"\\"\\"
            scalar BigInt

            type CreateFilesMutationResponse {
              files: [File!]!
              info: CreateInfo!
            }

            type CreateInfo {
              bookmark: String
              nodesCreated: Int!
              relationshipsCreated: Int!
            }

            type DeleteInfo {
              bookmark: String
              nodesDeleted: Int!
              relationshipsDeleted: Int!
            }

            type File {
              name: String!
              size: BigInt!
            }

            input FileCreateInput {
              name: String!
              size: BigInt!
            }

            input FileOptions {
              limit: Int
              offset: Int
              \\"\\"\\"Specify one or more FileSort objects to sort Files by. The sorts will be applied in the order in which they are arranged in the array.\\"\\"\\"
              sort: [FileSort]
            }

            \\"\\"\\"Fields to sort Files by. The order in which sorts are applied is not guaranteed when specifying many fields in one FileSort object.\\"\\"\\"
            input FileSort {
              name: SortDirection
              size: SortDirection
            }

            input FileUpdateInput {
              name: String
              size: BigInt
            }

            input FileWhere {
              AND: [FileWhere!]
              OR: [FileWhere!]
              name: String
              name_CONTAINS: String
              name_ENDS_WITH: String
              name_IN: [String]
              name_NOT: String
              name_NOT_CONTAINS: String
              name_NOT_ENDS_WITH: String
              name_NOT_IN: [String]
              name_NOT_STARTS_WITH: String
              name_STARTS_WITH: String
              size: BigInt
              size_GT: BigInt
              size_GTE: BigInt
              size_IN: [BigInt]
              size_LT: BigInt
              size_LTE: BigInt
              size_NOT: BigInt
              size_NOT_IN: [BigInt]
            }

            type Mutation {
              createFiles(input: [FileCreateInput!]!): CreateFilesMutationResponse!
              deleteFiles(where: FileWhere): DeleteInfo!
              updateFiles(update: FileUpdateInput, where: FileWhere): UpdateFilesMutationResponse!
            }

            type Query {
              files(options: FileOptions, where: FileWhere): [File!]!
              filesCount(where: FileWhere): Int!
            }

            enum SortDirection {
              \\"\\"\\"Sort by field values in ascending order.\\"\\"\\"
              ASC
              \\"\\"\\"Sort by field values in descending order.\\"\\"\\"
              DESC
            }

            type UpdateFilesMutationResponse {
              files: [File!]!
              info: UpdateInfo!
            }

            type UpdateInfo {
              bookmark: String
              nodesCreated: Int!
              nodesDeleted: Int!
              relationshipsCreated: Int!
              relationshipsDeleted: Int!
            }
            "
        `);
    });
});

Sorry about the hassle! PR reviews can be a bit slow when we are in the swing of our development phase, but we want to get this reviewed and merged following this update. 🙂

[dmoree]

@darrellwarde Fixed in 8a4600f.

No worries (I'm going to assume a copy/paste error was made). This change to snapshots will be for the better. Thanks.

[oskarhane]

This looks good, thanks for a great contribution (again)!
Much appreciated ⭐

[darrellwarde]

Brilliant, looks good, let's get this merged before we make any other breaking changes! 😆 Thanks @dmoree! 🙂