Skip to content
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

PatternJsonProvider: provide a #tryLong and #tryDouble operation similar to #tryJson #694

Open
brenuart opened this issue Nov 3, 2021 · 5 comments

Comments

@brenuart
Copy link
Collaborator

brenuart commented Nov 3, 2021

It may be handy to provide #tryLong and #tryDouble operations similar to #tryJson.
These two new operations would return the input (string) value when the conversion fails instead of null.

@brenuart
Copy link
Collaborator Author

brenuart commented Nov 5, 2021

Returning the input value (that is a string) does not make much sense for operations whose purpose is to convert to a type other than string. Instead, we could provide a way to specify the default value to use when the conversion fails.

For instance:

  • explicit default value
#asLong{...}:-<default value>
#asJson{...}:-<default value>

<default value> will be used when #asLong{} fails to convert the input. It must be convertible into the target type (long in this case) or an error is raised at configuration time.

  • use null
#asLong{...}:?
#asJson{...}:?

null is returned when the conversion fails - this is the default behaviour, similar to #asLong{...}.

  • leave input unchanged
#asLong{...}:-
#asJson{...}:-

Return the input string unchanged - same behaviour as #tryJson{...}. Can be used only with operations returning a String - will not be accepted for #asLong{} for instance...

This syntax will be preferred over the try..{} form. The latter will still be supported but slowly deprecated...

@philsttr your opinion?

@philsttr
Copy link
Collaborator

philsttr commented Nov 5, 2021

👍 Sounds good to me

@brenuart
Copy link
Collaborator Author

brenuart commented Nov 6, 2021

I made a mistake when describing the behaviour of the :- modifier. It is supposed to be an alternative to the try..{} variants. It should therefore always return the input string when the conversion fails - even when it does not match the return type of the operation. This is already the case for #tryJson{}: it may return a string instead of a JSON object...

However, I'm questioning the utility of this feature: the type of the field is not constant - sometimes a long (or an json object), sometimes a string. This can make consumption of the JSON output at the receiving side challenging.

I'm wondering if a better alternative would be to output the original string value in another field whose name is derived from the original. Suppose the following pattern:

{
   "numericValue": "#asLong(foo)"
}

foo is not a valid Long representation and causes #asLong{} to fail at runtime. This would produce something like this at runtime:

{
   "numericValue": null,
   "numericValue_invalid": "foo"
}

or

{
   "numericValue": null,
   "$numericValue": "foo"
}

or

{
   "numericValue": null,
   "@errors": {
      "numericValue": "foo"
   }
}

Fields declared in the pattern keep their desired type. Furthermore, errors are clearly identifiable and the faulty value is available for later post processing.

@brenuart
Copy link
Collaborator Author

brenuart commented Nov 6, 2021

Now that I think of it... this kind of mechanism could benefit to all JsonProviders. Instead of silently ignoring errors, JsonProviders could add an entry describing their issue under a special root field (@errors in the example above).

@philsttr
Copy link
Collaborator

philsttr commented Nov 7, 2021

I'm wondering if a better alternative would be to output the original string value in another field whose name is derived from the original.

I really like this idea.

Now that I think of it... this kind of mechanism could benefit to all JsonProviders. Instead of silently ignoring errors, JsonProviders could add an entry describing their issue under a special root field (@errors in the example above).

Since logback already has a generic mechanism for reporting errors (via StatusManager), I'm hesitant to add another. If you feel strongly that having all errors reported inline would be best, I'd like to see this idea more fully spec'ed out, since the simple example for @errors above is not generic enough for all types of errors.

I think a targetted solution for writing un-parsable values as a separate string field is sufficient.
I like the _INVALID suffix. The $ prefix seems likely to already have meaning in some systems. Perhaps even provide the ability to customize via a regex replacement string, with \1_INVALID being the default.
This new behavior would need to be able to be disabled to provide backwards compatibility.

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

No branches or pull requests

2 participants