feat: add SecondsSinceEpochScalar #163
Conversation
| static { | ||
| Coercing<TemporalAccessor, String> coercing = new Coercing<>() { | ||
| @Override | ||
| public String serialize(Object input, GraphQLContext graphQLContext, Locale locale) throws CoercingSerializeException { |
There was a problem hiding this comment.
Hmmm should this serialise as a String or a Number
The client is going to get a scalar value like
"fieldFoo" : "1750211141922"
if it tries to use it to compute anything compared to another epoch then it has to turn it into a number. for example in JavaScript Data.now() returns a number
I think this should always serialise as a number and not a string.
Why do you think it should be a String?
There was a problem hiding this comment.
I took our production code and added exceptions and handling of some corner cases. Based on git history, it looks like it was just an initial implementation and JS side just dynamically converts in what it needs.
And I agree that Long should work better here. Will update PR with changes.
| } | ||
| if (input instanceof String) { | ||
| String string = (String) input; | ||
| if (string.matches("\\d+")) { |
There was a problem hiding this comment.
Can we make a isNumber(str) that more efficient than Regex matching. We want Scalars to be as fast as possible
That said I can see other code that does
} else if (isNumberIsh(input)) {
BigDecimal value;
try {
value = new BigDecimal(input.toString());
} catch (NumberFormatException e) {
return null;
}
try {
return value.longValueExact();
} catch (ArithmeticException e) {
return null;
}
and that feels kinda expensive - but progress over perfection
There was a problem hiding this comment.
Good catch, I forgot to replace it with something more performant. Usually I use apache commons to do that check - StringUtils.isNumeric or NumberUtils.isParsable, but there is no such dependency. And I prefer to avoid throwing exception, which is not a big problem here as we still need to throw exception at the end. I will update it to try-catch.
Regexp decreases performance, so there is more sense to rely on parse exceptions in that case. Refs: graphql-java#157
Refs: #157