Support i16 as additional integer type

[weiznich]

Doing this seems to be safe.
This is useful for mapping database entries of the corresponding type to a graphql schema.

[codecov-io]

Codecov Report

Merging #198 into master will increase coverage by <.01%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   89.88%   89.89%   +<.01%     
==========================================
  Files          96       96              
  Lines       17442    17468      +26     
==========================================
+ Hits        15678    15702      +24     
- Misses       1764     1766       +2

Impacted Files	Coverage Δ
juniper/src/types/scalars.rs	81.2% <81.81%> (+2.69%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e841672...ab34b1d. Read the comment docs.

[LegNeato]

Hmmm, not sure if having two Int scalars will be a problem?

[weiznich]

Hmmm, not sure if having two Int scalars will be a problem?

I do not see any problem here because this is a thing that happens purely internally. Both types have exact the same representation exposed to clients. The only thing that could go possibly wrong is that someone tries to use a value outside of the valid range for i16, but this could also happen today (If someone tries to use a value outside of i32)

That said I think there should also be support for i64, and f32

[LegNeato]

Isn't the name "Int" name exposed in the introspection query? I could be turned around here...

[weiznich]

Isn't the name "Int" name exposed in the introspection query?

Yes sure, both are exposed as "Int" there, but I do not see any problem with this.
The only possibility that I see where something could go wrong is the case where someone tries to write a number bigger than i16 in a field exposed as integer. But the same problem also applies to the current implementation with i32.

To state it somewhat different: For the reading case this will work in every case, because the number is simply returned as number. For the writing case this will work as long as the supplied number is part of the value range of an i16 otherwise an error is returned.

[LegNeato]

Right, but the spec explicitly says Ints are 32bit:

graphql/graphql-js#292

So it would be completely unexpected for a client to provide an Int larger than i16 and get back an error (from the client perspective).

[weiznich]

So if we name that type just Int16 instead of Int everything will be fine?

[LegNeato]

Ok, after your explanation making it click (thanks!) and thinking about it some more, I don't think this belongs in Juniper. Juniper tries to be 100% spec compliant. In this case, the inbuilt scalars are defined in GraphQL and it wouldn't be correct to add a juniper-specific Int16...that would be juniper-specific extended functionality and may not be what library users expect. And as I said in the comments, merely allowing Int to be i16 isn't great as the spec explicitly says Ints are i32. It would be completely unexpected for a client to provide an Int larger than i16 and get back an error (from the client perspective) and thus Juniper wouldn't really be spec compliant.

If this is a large hardship for wundergraph rather than an annoyance it would be good to know to think through alternatives or perhaps add a feature flag to Juniper with this patch.

[weiznich]

If this is a large hardship for wundergraph rather than an annoyance it would be good to know to think through alternatives or perhaps add a feature flag to Juniper with this patch.

For wundergraph the situation is as follows:

• diesel maps database types to the corresponding rust types. This can mostly seen as 1:1 mapping. This means a SmallInt column maps only to a i16 field, a BigInt column maps only to a i64field.
• juniper only exposes i32, but those other two cases are quite common in the database world.

So there are possible several solutions here:

• diesel changes it's mind about how to map those types -> unlikely because this mappings are the only mappings that are technically correct
• there is some way to directly expose those types in some form through juniper in graphql api -> juniper is not anymore standard conform
• wundergraph does some mapping in between. In theory this is possible but this means that wundergraph needs to implement both diesel and juniper traits for some types. The types in question here are primitive types, so wundergraph does not own anything and cannot implement them straight forward. It is possible to provide new type mappers for those types, but in my opinion this makes the api a lot more unreadable.

That said: I'm on vacation for the next few weeks. Maybe I've a other idea there. Otherwise I will try to do some research on this as soon as I'm back. (I think we are not the first ones with this issue 😉. Someone out there surly needed to use for example 64 bit integers.)

[weiznich]

That said: I'm on vacation for the next few weeks. Maybe I've a other idea there.

As promised I've done some research on this.

Juniper tries to be 100% spec compliant. In this case, the inbuilt scalars are defined in GraphQL and it wouldn't be correct to add a juniper-specific Int16...that would be juniper-specific extended functionality and may not be what library users expect.

If I read the spec correctly it is totally OK to define custom scalar types:

GraphQL provides a number of built‐in scalars, but type systems can add additional scalars with semantic meaning. For example, a GraphQL system could define a scalar called Time which, while serialized as a string, promises to conform to ISO‐8601. When querying a field of type Time, you can then rely on the ability to parse the result with an ISO‐8601 parser and use a client‐specific primitive for time. Another example of a potentially useful custom scalar is Url, which serializes as a string, but is guaranteed by the server to be a valid URL.

source

And also the following section regarding to 64 bit integers (highlighting by me):

Numeric integer values larger than 32‐bit should either use String or a custom‐defined Scalar type, as not all platforms and transports support encoding integer numbers larger than 32‐bit.

soucre

Therefore I suggest the following solution:
Basically we define new scalar types for: i16, i64, f32 and possibly more types. (Due to the orphan rule this needs to be done in juniper. In theory I could workaround this by using new type wrappers in wundergraph but this would lead to quite ugly code)

ping @LegNeato @theduke
(Sidenote: This is the only remaining open issue for wundergraph on a third party project)