New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce 'disableLosslessIntegers' config option #323

Merged
merged 5 commits into from Feb 2, 2018

Conversation

Projects
None yet
7 participants
@lutovich
Copy link
Contributor

lutovich commented Jan 17, 2018

This boolean option allows users to make driver accept and represent integer values as native JavaScript Numbers instead of driver's special Integers. When this option is set to true driver will fail to encode Integer values passed as query parameters. All returned Record, Node, Relationship, etc. will have their integer fields as native Numbers. Object returned by Record#toObject() will also contain native numbers.

Warning: enabling this setting can potentially make driver return lossy integer values. This would be the case when database contain integer values which do not fit in [Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER] range. Such integers will then be represented as Number.NEGATIVE_INFINITY or Number.POSITIVE_INFINITY, which is lossy and different from what is actually stored in the database. That is why this option is set to false by default. Driver's Integer class is able to represent integer values beyond [Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER] and thus is a much safer option in the general case.

Fixes #322
Related to #245, #225, #122

Introduce 'useNativeNumbers' config option
This boolean option allows users to make driver accept and represent
integer values as native JavaScript `Number`s instead of driver's
special `Integer`s. When this option is set to `true` driver will
fail to encode `Integer` values passed as query parameters. All
returned `Record`, `Node`, `Relationship`, etc. will have their
integer fields as native `Number`s. Object returned by `Record#toObject()`
will also contain native numbers.

**Warning:** enabling this setting can potentially make driver return
lossy integer values. This would be the case when database contain
integer values which do not fit in
`[Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER]` range. Such
integers will then be represented as `Number.NEGATIVE_INFINITY` or
`Number.POSITIVE_INFINITY`, which is lossy and different from what is
actually stored in the database. That is why this option is set to
`false` by default. Driver's `Integer` class is able to represent
integer values beyond `[Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER]`
and thus is a much safer option in the general case.

@lutovich lutovich requested review from technige , oskarhane and ali-ince Jan 17, 2018

@lutovich

This comment has been minimized.

Copy link
Contributor

lutovich commented Jan 17, 2018

I'm unsure about two things:

  1. Is useNativeNumbers a good name for this setting? @nigelsmall could you please assess this?

  2. What to do with TypeScript declaration files? This setting basically changes what kind of type driver uses for integer values. So TS type info depends on this setting. Things like:

    declare class Node {
      identity: Integer;
      ...
    }

    will have to change somehow to represent this. Maybe it's enough to use union types like this:

    declare class Node {
      identity: Integer | number;
      ...
    }

    and add comments? @pocesar, @TimothyDespair, @cojack could you please suggest?

@ali-ince

This comment has been minimized.

Copy link
Contributor

ali-ince commented Jan 18, 2018

As per your points;

  1. I would prefer forceNativeNumbers because of its emphasise on forcing something that could go wrong (be lossy).

  2. AFAIK, we can either specify any or union types as you've already suggested. Going with union types feel the way to go.

@ali-ince
Copy link
Contributor

ali-ince left a comment

Changes look good to me.

What would you think about adding an toObject() overload which will convert Integer instances to number types although Driver is not configured to use native numbers?

@oskarhane
Copy link
Member

oskarhane left a comment

This looks good to me as well.

And I agree with the changes that if you opt out of using the integer type when receiving results you should also not send integer types as params.

@oskarhane

This comment has been minimized.

Copy link
Member

oskarhane commented Jan 19, 2018

btw, I'm +1 on @ali-ince param name suggestion forceNativeNumbers

@eelsweb

This comment has been minimized.

Copy link

eelsweb commented Jan 20, 2018

agree with what @ali-ince has suggested - this config option could also be received by Record.toObject() (and maybe also by Record.get())

Lets say majority of my cases can be handled by JS Numbers and in a few cases I need the Integer -

constructor -
neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"), {forceJSNumbers: true})
handle edge case with the same driver
record.toObject({forceJSNumbers: false})

If, its the other way around,

constructor -
neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"))
handle smaller cases with the same driver
record.toObject({forceJSNumbers: true})

also, the param could specify that we are forcing JS Numbers. Native might be confusing as in - native to javascript or native to neo4j - hence forceJSNumbers

@cojack

cojack approved these changes Jan 27, 2018

@pocesar

This comment has been minimized.

Copy link
Contributor

pocesar commented Jan 28, 2018

@lutovich you could make it a generic, so the person decides what type it should use, as such:

declare class Node<T = Integer> {
  identity: T;
  // ...
}

that means, if the person decides to not explicitly decide which type Node should be, it will be Integer (the default argument). If the person wants something else, he can then const node: Node<number> or type MyNode = Node<number>, and the types will be infered

lutovich added some commits Jan 29, 2018

Renamed 'useNativeNumbers' to 'disableLosslessIntegers'
New name seems more descriptive and emphasizes that numbers returned
from driver with this setting set to `true` might result in loss
of precision.
Allow storing of Integer when 'disableLosslessIntegers=true'
`Integer` will now be allowed in query parameters even when native JS
numbers are configured on the driver level. This is done to not cut
away a valid neo4j integer type. It will also result in better
backwards compatibility - existing applications will not need to
change all `Integer` query parameters when they
set `disableLosslessIntegers=true`.
@lutovich

This comment has been minimized.

Copy link
Contributor

lutovich commented Jan 29, 2018

@pocesar thanks a lot for your reply! The only problem I see with Node<T = Integer> is that users will be able to use Node<string> and try to access identity as string, while it's in fact Integer | number.

Here is a possible solution:

declare class Node<T extends (Integer | number) = Integer> {
  identity: T;
  // ...
}

it's essentially the same thing but adds an additional generic type bound. So no code change for existing users, since Integer is still the default generic type. Users who enable JS numbers will have to use Node<number>.

What do you think?

TS declarations support for 'disableLosslessIntegers=true'
Previously TS declaration files only allowed returned integer types to
be `neo4j.Integer`. This was not compatible with newly introduced
`disableLosslessIntegers` config, which makes driver always return
native JS numbers.

This commit allows all integer properties to either be `neo4j.Integer`
or `number`. `Integer` is default. Existing TS users will not need to
change their code. TS users who set 'disableLosslessIntegers=true'
will have to specify generic type explicitly like `Node<number>`.
Property `identity` of such node will have type `number`.
@lutovich

This comment has been minimized.

Copy link
Contributor

lutovich commented Jan 29, 2018

PR updated and should now be ready for a final review. Changes:

  • renamed useNativeNumbers to disableLosslessIntegers
  • lifted restriction on storing Integer when disableLosslessIntegers=true because it allows for better backwards compatibility
  • updated TS declarations to support number instead of Integer

See commit messages for more details.

@eelsweb we've decided to only expose disableLosslessIntegers config on the driver level for now. Having it on both driver and record level makes things more complicated. What if it's set to true on the driver level but user calls record.toObject({disableLosslessIntegers: false})? It's definitely implementable but weird.

This change will hopefully be merged in 1.6 and go through alpha, beta and RC stages. Feedback is very valuable!

@pocesar

This comment has been minimized.

Copy link
Contributor

pocesar commented Jan 29, 2018

@lutovich if the identity member will ALWAYS be either Integer or number, then it's the best choice to do that 👍
just a nit-pick, you can also explicitly decouple that from the declaration by creating a new type, like

declare type NumberInteger = Integer | number;
declare class Node<T extends NumberInteger = Integer> {
}

so it can be reused elsewhere

Extract NumberOrInteger type in TS declarations
To deduplicate and reuse `(number | Integer)` generic type declaration.
@eelsweb

This comment has been minimized.

Copy link

eelsweb commented Feb 2, 2018

@lutovich this kind of over-riding is actually quite standard practice

For instance - Google's Nodejs Lib for their APIs

View the section Options

You may specify additional options either in the global google object or on a service client basis.

You can specify an auth object to be used per request. Each request also inherits the options specified at the service level and global level.

For this neo4j library, consider a case where 80 pc of my functions work with disableLosslessIntegers=true, but there are some cases where i need Integer, so I create a driver with {disableLosslessIntegers:true} and for the remaining few cases, I don't need a separate driver, I can simply override that by calling toObject({disableLosslessIntegers: false}) since local config takes precedence over global options.

@lutovich

This comment has been minimized.

Copy link
Contributor

lutovich commented Feb 2, 2018

@eelsweb is this an actual use-case you have or just a theoretical concern?

Imho disableLosslessIntegers is a good first step. It solves the problem many people complained about. Overrides on the Record level are definitely doable but will make API more complicated. It is now unclear if they are needed or solve real problems. That is why I think we can defer them for future, at least right now.

@eelsweb

This comment has been minimized.

Copy link

eelsweb commented Feb 2, 2018

Agreed 😄

@zhenlineo zhenlineo merged commit 020be65 into neo4j:1.6 Feb 2, 2018

1 check passed

js-driver-pull-requests (driver PR builds and benchmarks, tools) - merge TeamCity build finished
Details

@lutovich lutovich deleted the lutovich:1.6-native-numbers branch Feb 2, 2018

@lutovich lutovich changed the title Introduce 'useNativeNumbers' config option Introduce 'disableLosslessIntegers' config option Feb 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment