You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@johnzon.apache.org by James Carman <ja...@carmanconsulting.com> on 2019/01/01 15:31:10 UTC
Inconsistent Mapping Behavior...
I am looking at JOHNZON-177 and I have noticed that we seem to be mapping
primitives inconsistently inside the same class.
MappingParserImpl.toObject() and MappingParserImpl.readObject() both
contain special logic for handling longs and ints. Why wouldn't we want to
handle these types consistently regardless of where they're used (fields or
the root type)?
I have included code to handle the overflow/underflow case with a nice
error message and that works fine when I added tests to the
DefaultMappingTest from the JSON-B module. However, I found that
MapperTest seems to already have similar tests for invalid long/int
values. As I trace the logic, it doesn't hit my new code.
Re: Inconsistent Mapping Behavior...
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Do it, it is just on my fork for now, no issue to not push it myself ;)
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mer. 2 janv. 2019 à 16:49, James Carman <ja...@carmanconsulting.com> a
écrit :
> Yes, but I was kinda hoping to do it myself. I’ve not been contributing as
> much as I’d like. Good work!
>
> On Wed, Jan 2, 2019 at 4:24 AM Romain Manni-Bucau <rm...@gmail.com>
> wrote:
>
> > Hi James and guys,
> >
> > Is
> >
> >
> https://github.com/rmannibucau/johnzon/commit/23dfc58e301fb87bb72bc8bb4cdeb16d05666d4e
> > what you had in mind?
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > <https://rmannibucau.metawerx.net/> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github <
> > https://github.com/rmannibucau> |
> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> > <
> >
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> > >
> >
> >
> > Le mar. 1 janv. 2019 à 22:16, Romain Manni-Bucau <rm...@gmail.com>
> a
> > écrit :
> >
> > > Agree, we just nees to ensure to not expose more than today internals
> > > probably but i dont see any blockers :)
> > >
> > > Le mar. 1 janv. 2019 17:26, James Carman <ja...@carmanconsulting.com>
> a
> > > écrit :
> > >
> > >> Well, ideally, the recursive one and the root one would use the same
> > logic
> > >> to map a JsonValue/JsonNumber -> Integer object, right? Perhaps we
> need
> > >> to
> > >> converge all this together and save ourselves some code.
> > >>
> > >> On Tue, Jan 1, 2019 at 11:19 AM Romain Manni-Bucau <
> > rmannibucau@gmail.com
> > >> >
> > >> wrote:
> > >>
> > >> > Hi James
> > >> >
> > >> > I likely need test cases to understand more the issue but long story
> > >> short
> > >> > one of the two method is recursive not the other so one must handle
> > >> > primitives as root types and the other aq nested type. Historically
> > >> > primitives were not possible root types so can be something to
> refind.
> > >> >
> > >> > Happy to review Jira+pr to be more accurate if needed.
> > >> >
> > >> > Le mar. 1 janv. 2019 16:31, James Carman <
> james@carmanconsulting.com>
> > a
> > >> > écrit :
> > >> >
> > >> > > I am looking at JOHNZON-177 and I have noticed that we seem to be
> > >> mapping
> > >> > > primitives inconsistently inside the same class.
> > >> > > MappingParserImpl.toObject() and MappingParserImpl.readObject()
> both
> > >> > > contain special logic for handling longs and ints. Why wouldn't
> we
> > >> want
> > >> > to
> > >> > > handle these types consistently regardless of where they're used
> > >> (fields
> > >> > or
> > >> > > the root type)?
> > >> > >
> > >> > > I have included code to handle the overflow/underflow case with a
> > nice
> > >> > > error message and that works fine when I added tests to the
> > >> > > DefaultMappingTest from the JSON-B module. However, I found that
> > >> > > MapperTest seems to already have similar tests for invalid
> long/int
> > >> > > values. As I trace the logic, it doesn't hit my new code.
> > >> > >
> > >> >
> > >>
> > >
> >
>
Re: Inconsistent Mapping Behavior...
Posted by James Carman <ja...@carmanconsulting.com>.
Yes, but I was kinda hoping to do it myself. I’ve not been contributing as
much as I’d like. Good work!
On Wed, Jan 2, 2019 at 4:24 AM Romain Manni-Bucau <rm...@gmail.com>
wrote:
> Hi James and guys,
>
> Is
>
> https://github.com/rmannibucau/johnzon/commit/23dfc58e301fb87bb72bc8bb4cdeb16d05666d4e
> what you had in mind?
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
>
>
> Le mar. 1 janv. 2019 à 22:16, Romain Manni-Bucau <rm...@gmail.com> a
> écrit :
>
> > Agree, we just nees to ensure to not expose more than today internals
> > probably but i dont see any blockers :)
> >
> > Le mar. 1 janv. 2019 17:26, James Carman <ja...@carmanconsulting.com> a
> > écrit :
> >
> >> Well, ideally, the recursive one and the root one would use the same
> logic
> >> to map a JsonValue/JsonNumber -> Integer object, right? Perhaps we need
> >> to
> >> converge all this together and save ourselves some code.
> >>
> >> On Tue, Jan 1, 2019 at 11:19 AM Romain Manni-Bucau <
> rmannibucau@gmail.com
> >> >
> >> wrote:
> >>
> >> > Hi James
> >> >
> >> > I likely need test cases to understand more the issue but long story
> >> short
> >> > one of the two method is recursive not the other so one must handle
> >> > primitives as root types and the other aq nested type. Historically
> >> > primitives were not possible root types so can be something to refind.
> >> >
> >> > Happy to review Jira+pr to be more accurate if needed.
> >> >
> >> > Le mar. 1 janv. 2019 16:31, James Carman <ja...@carmanconsulting.com>
> a
> >> > écrit :
> >> >
> >> > > I am looking at JOHNZON-177 and I have noticed that we seem to be
> >> mapping
> >> > > primitives inconsistently inside the same class.
> >> > > MappingParserImpl.toObject() and MappingParserImpl.readObject() both
> >> > > contain special logic for handling longs and ints. Why wouldn't we
> >> want
> >> > to
> >> > > handle these types consistently regardless of where they're used
> >> (fields
> >> > or
> >> > > the root type)?
> >> > >
> >> > > I have included code to handle the overflow/underflow case with a
> nice
> >> > > error message and that works fine when I added tests to the
> >> > > DefaultMappingTest from the JSON-B module. However, I found that
> >> > > MapperTest seems to already have similar tests for invalid long/int
> >> > > values. As I trace the logic, it doesn't hit my new code.
> >> > >
> >> >
> >>
> >
>
Re: Inconsistent Mapping Behavior...
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi James and guys,
Is
https://github.com/rmannibucau/johnzon/commit/23dfc58e301fb87bb72bc8bb4cdeb16d05666d4e
what you had in mind?
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mar. 1 janv. 2019 à 22:16, Romain Manni-Bucau <rm...@gmail.com> a
écrit :
> Agree, we just nees to ensure to not expose more than today internals
> probably but i dont see any blockers :)
>
> Le mar. 1 janv. 2019 17:26, James Carman <ja...@carmanconsulting.com> a
> écrit :
>
>> Well, ideally, the recursive one and the root one would use the same logic
>> to map a JsonValue/JsonNumber -> Integer object, right? Perhaps we need
>> to
>> converge all this together and save ourselves some code.
>>
>> On Tue, Jan 1, 2019 at 11:19 AM Romain Manni-Bucau <rmannibucau@gmail.com
>> >
>> wrote:
>>
>> > Hi James
>> >
>> > I likely need test cases to understand more the issue but long story
>> short
>> > one of the two method is recursive not the other so one must handle
>> > primitives as root types and the other aq nested type. Historically
>> > primitives were not possible root types so can be something to refind.
>> >
>> > Happy to review Jira+pr to be more accurate if needed.
>> >
>> > Le mar. 1 janv. 2019 16:31, James Carman <ja...@carmanconsulting.com> a
>> > écrit :
>> >
>> > > I am looking at JOHNZON-177 and I have noticed that we seem to be
>> mapping
>> > > primitives inconsistently inside the same class.
>> > > MappingParserImpl.toObject() and MappingParserImpl.readObject() both
>> > > contain special logic for handling longs and ints. Why wouldn't we
>> want
>> > to
>> > > handle these types consistently regardless of where they're used
>> (fields
>> > or
>> > > the root type)?
>> > >
>> > > I have included code to handle the overflow/underflow case with a nice
>> > > error message and that works fine when I added tests to the
>> > > DefaultMappingTest from the JSON-B module. However, I found that
>> > > MapperTest seems to already have similar tests for invalid long/int
>> > > values. As I trace the logic, it doesn't hit my new code.
>> > >
>> >
>>
>
Re: Inconsistent Mapping Behavior...
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Agree, we just nees to ensure to not expose more than today internals
probably but i dont see any blockers :)
Le mar. 1 janv. 2019 17:26, James Carman <ja...@carmanconsulting.com> a
écrit :
> Well, ideally, the recursive one and the root one would use the same logic
> to map a JsonValue/JsonNumber -> Integer object, right? Perhaps we need to
> converge all this together and save ourselves some code.
>
> On Tue, Jan 1, 2019 at 11:19 AM Romain Manni-Bucau <rm...@gmail.com>
> wrote:
>
> > Hi James
> >
> > I likely need test cases to understand more the issue but long story
> short
> > one of the two method is recursive not the other so one must handle
> > primitives as root types and the other aq nested type. Historically
> > primitives were not possible root types so can be something to refind.
> >
> > Happy to review Jira+pr to be more accurate if needed.
> >
> > Le mar. 1 janv. 2019 16:31, James Carman <ja...@carmanconsulting.com> a
> > écrit :
> >
> > > I am looking at JOHNZON-177 and I have noticed that we seem to be
> mapping
> > > primitives inconsistently inside the same class.
> > > MappingParserImpl.toObject() and MappingParserImpl.readObject() both
> > > contain special logic for handling longs and ints. Why wouldn't we
> want
> > to
> > > handle these types consistently regardless of where they're used
> (fields
> > or
> > > the root type)?
> > >
> > > I have included code to handle the overflow/underflow case with a nice
> > > error message and that works fine when I added tests to the
> > > DefaultMappingTest from the JSON-B module. However, I found that
> > > MapperTest seems to already have similar tests for invalid long/int
> > > values. As I trace the logic, it doesn't hit my new code.
> > >
> >
>
Re: Inconsistent Mapping Behavior...
Posted by James Carman <ja...@carmanconsulting.com>.
Well, ideally, the recursive one and the root one would use the same logic
to map a JsonValue/JsonNumber -> Integer object, right? Perhaps we need to
converge all this together and save ourselves some code.
On Tue, Jan 1, 2019 at 11:19 AM Romain Manni-Bucau <rm...@gmail.com>
wrote:
> Hi James
>
> I likely need test cases to understand more the issue but long story short
> one of the two method is recursive not the other so one must handle
> primitives as root types and the other aq nested type. Historically
> primitives were not possible root types so can be something to refind.
>
> Happy to review Jira+pr to be more accurate if needed.
>
> Le mar. 1 janv. 2019 16:31, James Carman <ja...@carmanconsulting.com> a
> écrit :
>
> > I am looking at JOHNZON-177 and I have noticed that we seem to be mapping
> > primitives inconsistently inside the same class.
> > MappingParserImpl.toObject() and MappingParserImpl.readObject() both
> > contain special logic for handling longs and ints. Why wouldn't we want
> to
> > handle these types consistently regardless of where they're used (fields
> or
> > the root type)?
> >
> > I have included code to handle the overflow/underflow case with a nice
> > error message and that works fine when I added tests to the
> > DefaultMappingTest from the JSON-B module. However, I found that
> > MapperTest seems to already have similar tests for invalid long/int
> > values. As I trace the logic, it doesn't hit my new code.
> >
>
Re: Inconsistent Mapping Behavior...
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi James
I likely need test cases to understand more the issue but long story short
one of the two method is recursive not the other so one must handle
primitives as root types and the other aq nested type. Historically
primitives were not possible root types so can be something to refind.
Happy to review Jira+pr to be more accurate if needed.
Le mar. 1 janv. 2019 16:31, James Carman <ja...@carmanconsulting.com> a
écrit :
> I am looking at JOHNZON-177 and I have noticed that we seem to be mapping
> primitives inconsistently inside the same class.
> MappingParserImpl.toObject() and MappingParserImpl.readObject() both
> contain special logic for handling longs and ints. Why wouldn't we want to
> handle these types consistently regardless of where they're used (fields or
> the root type)?
>
> I have included code to handle the overflow/underflow case with a nice
> error message and that works fine when I added tests to the
> DefaultMappingTest from the JSON-B module. However, I found that
> MapperTest seems to already have similar tests for invalid long/int
> values. As I trace the logic, it doesn't hit my new code.
>