johnzon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Romain Manni-Bucau <rmannibu...@gmail.com>
Subject Re: Improving "Missing @JsonbCreator argument" message
Date Thu, 23 Jul 2020 06:35:30 GMT
Le jeu. 23 juil. 2020 à 08:02, Arne Limburg <arne.limburg@openknowledge.de>
a écrit :

> Hi Romain,
>
>
> This will happen in production regularly when a clients sends wrong
> payload (you can't expect that not to happen).
>

Yes and no, only for buggy code.
Basically this must not happen cause jsonb is not the one which should do
the validation, this is why we can disable the spec requirement to enable
business messages.
A technical message is wrong anyway and not i18n.

Indeed does not mean we must not enhance it, we must, but maybe we should
document to not use that default behavior too?


> Personally I would go for option 5:
>
>
> Add a functional interface FactoryValidator, that has a validate method
> with three parameters (class, method, args) and replace the Consumer with
> that interface
>
>
> It's litte code change and satisfies the need.
>
>
> Just my two cent,
>
> Cheers,
>
> Arne
>
>
> OPEN KNOWLEDGE GmbH
> Poststraße 1, 26122 Oldenburg
> Mobil: +49 151 - 108 22 942
> Tel: +49 441 - 4082-154
> Fax: +49 441 - 4082-111
> arne.limburg@openknowledge.de
> www.openknowledge.de <https://www.openknowledge.de/>
>
> Registergericht: Amtsgericht Oldenburg, HRB 4670
> Geschäftsführer: Lars Röwekamp, Jens Schumann
>
> Treffen Sie uns auf kommenden Konferenzen und Workshops:
>
> Zu unseren Events<https://www.openknowledge.de/event/>
>
>
>
>
>
> ________________________________
> Von: Romain Manni-Bucau <rmannibucau@gmail.com>
> Gesendet: Donnerstag, 23. Juli 2020 07:17
> An: dev@johnzon.apache.org
> Betreff: Re: Improving "Missing @JsonbCreator argument" message
>
> Hi David,
>
> Good catch - looks like a too fast "make it pass the tck" ;)
>
> A few comments inline.
>
> Le jeu. 23 juil. 2020 à 04:40, David Blevins <david.blevins@gmail.com> a
> écrit :
>
> > Hey All!
> >
> > I'd really like to improve the above error message, but I would like to
> do
> > it in a way that has the highest chance to be merged so first asking for
> > guidance.
> >
> > Ideally the message would tell me:
> >
> >  1) the class annotated with @JsonbCreator
> >  2) the arguments that are missing
> >
>
> +1
>
>
> > A very optional, but would be slick:
> >
> >  3) that the `johnzon.failOnMissingCreatorValues` flag exists
> >
>
> Maybe more "you didnt set this flag and params are missing" in case the dev
> knows about it (rather than "it exists")?
>
>
> > The most obvious trick to this is that the validation is done in a lambda
> > here:
> >
> >  -
> >
> https://github.com/apache/johnzon/blob/master/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbAccessMode.java#L223
> >
> > However the data we would need to produce an informative message is here:
> >
> >  -
> >
> https://github.com/apache/johnzon/blob/master/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbAccessMode.java#L322
> >
> > This is me brainstorming as I type, but I can imagine a few ways this
> > could be solved:
> >
> >  1) Move the lambda down to after the `if` block that sets `final
> String[]
> > params`, then we have what we need to build a better message.
> >
> >  2) Replace the lambda `factoryValidator.accept(params)` with a plain
> > method call `validateFactory(clazz, params, args)`.  We'd need to rename
> > `public Object create(final Object[] params)` to public Object
> create(final
> > Object[] args)` so there's no conflict with the `String[] params` in the
> > outter method signature.
> >
> >  3) Make the `factoryValidator` lambda throw a subclass of JsonbException
> > that has a field indicating the `args` array positions that are null.
> Then
> > make `public Object create(final Object[] params)` method catch it, match
> > up the missing arg positions with the names and wrap the exception with a
> > more detailed exception.
> >
> >  4) Change the lambda from `final Consumer<Object[]> factoryValidator` to
> > something like `final Consumer<FactoryMetadata> factoryValidator` and
> > introduce a small inner class FactoryMetadata that has clazz, params,
> args
> > as fields.
> >
>
> Think I'd just catch the exception - we know its type and only exception
> which can be thrown - and recreate it as needed. This is not a case which
> will happen in prod until there is a bug so should be good enough and avoid
> new classes or breaking changes.
>
>
> > Possibly some other options jump out at others.
> >
> > If I don't hear anything, I'll pick one and see how it goes.  While I'm
> at
> > it I'd like to improve the other error messages in the area so at minimum
> > they say the affected class:
> >
> >  -
> >
> https://github.com/apache/johnzon/blob/master/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbAccessMode.java#L192
> >  -
> >
> https://github.com/apache/johnzon/blob/master/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbAccessMode.java#L206
> >  -
> >
> https://github.com/apache/johnzon/blob/master/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbAccessMode.java#L292
>
>
> +1
>
>
> >
> >
> >
> >
> >
> >
> > -David
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message