You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Dan Smith <ds...@pivotal.io> on 2019/05/14 17:40:50 UTC
Backwards compatibility issue with JSONFormatter
I think the changes for GEODE-6327 broke backwards compatibility in
JSONFormatter with the change from fromJSON(String jsonString) to
fromJSON(String jsonString, String... identityFields)
Adding an additional varargs parameter to the method breaks code that was
compiled against the non-varargs version. I think we need to overload the
method instead.
Thanks to Gester for discovering this with his test!
-Dan
Re: Backwards compatibility issue with JSONFormatter
Posted by Jared Stewart <st...@gmail.com>.
It looks like the japi-compliance-checker tool to which Anthony linked is
available as a gradle plugin: https://github.com/melix/japicmp-gradle-plugin
On Tue, May 14, 2019 at 5:07 PM Ryan McMahon <rm...@pivotal.io> wrote:
> Fixed in
>
> https://github.com/apache/geode/commit/cc0b37a504480f554b1884491f44a3cca4113ef5
>
> On Tue, May 14, 2019 at 2:06 PM Ryan McMahon <rm...@pivotal.io> wrote:
>
> > Thanks Anthony! Have we considered using a compliance checker in our CI
> > like the one in the first link?
> >
> > Side note - I think that varargs is an interesting case that I don't see
> > called out in those links. Since varargs is just syntactic sugar, it is
> > ultimately the varargs parameter is converted to an array in the
> bytecode.
> > So in the JSON formatter change:
> >
> > fromJSON(byte[] jsonByteArray, String... identityFields)
> >
> >
> > becomes
> >
> > fromJSON(byte[] jsonByteArray, String[] identityFields)
> >
> >
> > Hence the breakage of the ABI.
> >
> > Ryan
> >
> > On Tue, May 14, 2019 at 1:33 PM Anthony Baker <ab...@pivotal.io> wrote:
> >
> >> Here are a few links on API compatibility:
> >>
> >> https://lvc.github.io/japi-compliance-checker/#Examples
> >> https://wiki.eclipse.org/Evolving_Java-based_APIs
> >> https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html
> >>
> >>
> >> Anthony
> >>
> >>
> >> > On May 14, 2019, at 12:45 PM, Dan Smith <ds...@pivotal.io> wrote:
> >> >
> >> > Sounds good! Yeah, this is a bit of an interesting case where code
> will
> >> > still compile against the new API without change, but I think
> >> maintaining
> >> > compatibility of the binary API is also important. Thanks for looking
> >> into
> >> > it.
> >> >
> >> > -Dan
> >> >
> >> > On Tue, May 14, 2019 at 11:22 AM Ryan McMahon <rm...@pivotal.io>
> >> wrote:
> >> >
> >> >> Hi all,
> >> >>
> >> >> This is my mistake - it was a misunderstanding of what constitutes a
> >> >> breaking public API change. If a client app were to recompile using
> >> the
> >> >> new bits with the new method signature, there wouldn't be an issue
> >> because
> >> >> the new signature would work with 0 varargs. The problem arises when
> >> you
> >> >> hot swap the geode dependencies for that client app without
> >> recompiling, as
> >> >> the bytecode no longer matches. Since we do support the hot swap use
> >> case
> >> >> for CVEs etc, I see now this is considered a breaking API change.
> >> >>
> >> >> I'll change this to use a method overload instead, it should be a
> >> pretty
> >> >> simple fix. Luckily, this issue didn't make it into any Geode
> >> releases.
> >> >>
> >> >> Thanks,
> >> >> Ryan
> >> >>
> >> >>
> >> >> On Tue, May 14, 2019 at 10:47 AM Udo Kohlmeyer <ud...@apache.org>
> wrote:
> >> >>
> >> >>> How did this slip the review process that the signature of a public
> >> API
> >> >>> was changed?
> >> >>>
> >> >>> Well done Gester for finding this!!
> >> >>>
> >> >>> --Udo
> >> >>>
> >> >>> On 5/14/19 10:40, Dan Smith wrote:
> >> >>>> I think the changes for GEODE-6327 broke backwards compatibility in
> >> >>>> JSONFormatter with the change from fromJSON(String jsonString) to
> >> >>>> fromJSON(String jsonString, String... identityFields)
> >> >>>>
> >> >>>> Adding an additional varargs parameter to the method breaks code
> that
> >> >> was
> >> >>>> compiled against the non-varargs version. I think we need to
> overload
> >> >> the
> >> >>>> method instead.
> >> >>>>
> >> >>>> Thanks to Gester for discovering this with his test!
> >> >>>>
> >> >>>> -Dan
> >> >>>>
> >> >>>
> >> >>
> >>
> >>
>
Re: Backwards compatibility issue with JSONFormatter
Posted by Ryan McMahon <rm...@pivotal.io>.
Fixed in
https://github.com/apache/geode/commit/cc0b37a504480f554b1884491f44a3cca4113ef5
On Tue, May 14, 2019 at 2:06 PM Ryan McMahon <rm...@pivotal.io> wrote:
> Thanks Anthony! Have we considered using a compliance checker in our CI
> like the one in the first link?
>
> Side note - I think that varargs is an interesting case that I don't see
> called out in those links. Since varargs is just syntactic sugar, it is
> ultimately the varargs parameter is converted to an array in the bytecode.
> So in the JSON formatter change:
>
> fromJSON(byte[] jsonByteArray, String... identityFields)
>
>
> becomes
>
> fromJSON(byte[] jsonByteArray, String[] identityFields)
>
>
> Hence the breakage of the ABI.
>
> Ryan
>
> On Tue, May 14, 2019 at 1:33 PM Anthony Baker <ab...@pivotal.io> wrote:
>
>> Here are a few links on API compatibility:
>>
>> https://lvc.github.io/japi-compliance-checker/#Examples
>> https://wiki.eclipse.org/Evolving_Java-based_APIs
>> https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html
>>
>>
>> Anthony
>>
>>
>> > On May 14, 2019, at 12:45 PM, Dan Smith <ds...@pivotal.io> wrote:
>> >
>> > Sounds good! Yeah, this is a bit of an interesting case where code will
>> > still compile against the new API without change, but I think
>> maintaining
>> > compatibility of the binary API is also important. Thanks for looking
>> into
>> > it.
>> >
>> > -Dan
>> >
>> > On Tue, May 14, 2019 at 11:22 AM Ryan McMahon <rm...@pivotal.io>
>> wrote:
>> >
>> >> Hi all,
>> >>
>> >> This is my mistake - it was a misunderstanding of what constitutes a
>> >> breaking public API change. If a client app were to recompile using
>> the
>> >> new bits with the new method signature, there wouldn't be an issue
>> because
>> >> the new signature would work with 0 varargs. The problem arises when
>> you
>> >> hot swap the geode dependencies for that client app without
>> recompiling, as
>> >> the bytecode no longer matches. Since we do support the hot swap use
>> case
>> >> for CVEs etc, I see now this is considered a breaking API change.
>> >>
>> >> I'll change this to use a method overload instead, it should be a
>> pretty
>> >> simple fix. Luckily, this issue didn't make it into any Geode
>> releases.
>> >>
>> >> Thanks,
>> >> Ryan
>> >>
>> >>
>> >> On Tue, May 14, 2019 at 10:47 AM Udo Kohlmeyer <ud...@apache.org> wrote:
>> >>
>> >>> How did this slip the review process that the signature of a public
>> API
>> >>> was changed?
>> >>>
>> >>> Well done Gester for finding this!!
>> >>>
>> >>> --Udo
>> >>>
>> >>> On 5/14/19 10:40, Dan Smith wrote:
>> >>>> I think the changes for GEODE-6327 broke backwards compatibility in
>> >>>> JSONFormatter with the change from fromJSON(String jsonString) to
>> >>>> fromJSON(String jsonString, String... identityFields)
>> >>>>
>> >>>> Adding an additional varargs parameter to the method breaks code that
>> >> was
>> >>>> compiled against the non-varargs version. I think we need to overload
>> >> the
>> >>>> method instead.
>> >>>>
>> >>>> Thanks to Gester for discovering this with his test!
>> >>>>
>> >>>> -Dan
>> >>>>
>> >>>
>> >>
>>
>>
Re: Backwards compatibility issue with JSONFormatter
Posted by Ryan McMahon <rm...@pivotal.io>.
Thanks Anthony! Have we considered using a compliance checker in our CI
like the one in the first link?
Side note - I think that varargs is an interesting case that I don't see
called out in those links. Since varargs is just syntactic sugar, it is
ultimately the varargs parameter is converted to an array in the bytecode.
So in the JSON formatter change:
fromJSON(byte[] jsonByteArray, String... identityFields)
becomes
fromJSON(byte[] jsonByteArray, String[] identityFields)
Hence the breakage of the ABI.
Ryan
On Tue, May 14, 2019 at 1:33 PM Anthony Baker <ab...@pivotal.io> wrote:
> Here are a few links on API compatibility:
>
> https://lvc.github.io/japi-compliance-checker/#Examples
> https://wiki.eclipse.org/Evolving_Java-based_APIs
> https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html
>
>
> Anthony
>
>
> > On May 14, 2019, at 12:45 PM, Dan Smith <ds...@pivotal.io> wrote:
> >
> > Sounds good! Yeah, this is a bit of an interesting case where code will
> > still compile against the new API without change, but I think maintaining
> > compatibility of the binary API is also important. Thanks for looking
> into
> > it.
> >
> > -Dan
> >
> > On Tue, May 14, 2019 at 11:22 AM Ryan McMahon <rm...@pivotal.io>
> wrote:
> >
> >> Hi all,
> >>
> >> This is my mistake - it was a misunderstanding of what constitutes a
> >> breaking public API change. If a client app were to recompile using the
> >> new bits with the new method signature, there wouldn't be an issue
> because
> >> the new signature would work with 0 varargs. The problem arises when
> you
> >> hot swap the geode dependencies for that client app without
> recompiling, as
> >> the bytecode no longer matches. Since we do support the hot swap use
> case
> >> for CVEs etc, I see now this is considered a breaking API change.
> >>
> >> I'll change this to use a method overload instead, it should be a pretty
> >> simple fix. Luckily, this issue didn't make it into any Geode releases.
> >>
> >> Thanks,
> >> Ryan
> >>
> >>
> >> On Tue, May 14, 2019 at 10:47 AM Udo Kohlmeyer <ud...@apache.org> wrote:
> >>
> >>> How did this slip the review process that the signature of a public API
> >>> was changed?
> >>>
> >>> Well done Gester for finding this!!
> >>>
> >>> --Udo
> >>>
> >>> On 5/14/19 10:40, Dan Smith wrote:
> >>>> I think the changes for GEODE-6327 broke backwards compatibility in
> >>>> JSONFormatter with the change from fromJSON(String jsonString) to
> >>>> fromJSON(String jsonString, String... identityFields)
> >>>>
> >>>> Adding an additional varargs parameter to the method breaks code that
> >> was
> >>>> compiled against the non-varargs version. I think we need to overload
> >> the
> >>>> method instead.
> >>>>
> >>>> Thanks to Gester for discovering this with his test!
> >>>>
> >>>> -Dan
> >>>>
> >>>
> >>
>
>
Re: Backwards compatibility issue with JSONFormatter
Posted by Anthony Baker <ab...@pivotal.io>.
Here are a few links on API compatibility:
https://lvc.github.io/japi-compliance-checker/#Examples
https://wiki.eclipse.org/Evolving_Java-based_APIs
https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html
Anthony
> On May 14, 2019, at 12:45 PM, Dan Smith <ds...@pivotal.io> wrote:
>
> Sounds good! Yeah, this is a bit of an interesting case where code will
> still compile against the new API without change, but I think maintaining
> compatibility of the binary API is also important. Thanks for looking into
> it.
>
> -Dan
>
> On Tue, May 14, 2019 at 11:22 AM Ryan McMahon <rm...@pivotal.io> wrote:
>
>> Hi all,
>>
>> This is my mistake - it was a misunderstanding of what constitutes a
>> breaking public API change. If a client app were to recompile using the
>> new bits with the new method signature, there wouldn't be an issue because
>> the new signature would work with 0 varargs. The problem arises when you
>> hot swap the geode dependencies for that client app without recompiling, as
>> the bytecode no longer matches. Since we do support the hot swap use case
>> for CVEs etc, I see now this is considered a breaking API change.
>>
>> I'll change this to use a method overload instead, it should be a pretty
>> simple fix. Luckily, this issue didn't make it into any Geode releases.
>>
>> Thanks,
>> Ryan
>>
>>
>> On Tue, May 14, 2019 at 10:47 AM Udo Kohlmeyer <ud...@apache.org> wrote:
>>
>>> How did this slip the review process that the signature of a public API
>>> was changed?
>>>
>>> Well done Gester for finding this!!
>>>
>>> --Udo
>>>
>>> On 5/14/19 10:40, Dan Smith wrote:
>>>> I think the changes for GEODE-6327 broke backwards compatibility in
>>>> JSONFormatter with the change from fromJSON(String jsonString) to
>>>> fromJSON(String jsonString, String... identityFields)
>>>>
>>>> Adding an additional varargs parameter to the method breaks code that
>> was
>>>> compiled against the non-varargs version. I think we need to overload
>> the
>>>> method instead.
>>>>
>>>> Thanks to Gester for discovering this with his test!
>>>>
>>>> -Dan
>>>>
>>>
>>
Re: Backwards compatibility issue with JSONFormatter
Posted by Dan Smith <ds...@pivotal.io>.
Sounds good! Yeah, this is a bit of an interesting case where code will
still compile against the new API without change, but I think maintaining
compatibility of the binary API is also important. Thanks for looking into
it.
-Dan
On Tue, May 14, 2019 at 11:22 AM Ryan McMahon <rm...@pivotal.io> wrote:
> Hi all,
>
> This is my mistake - it was a misunderstanding of what constitutes a
> breaking public API change. If a client app were to recompile using the
> new bits with the new method signature, there wouldn't be an issue because
> the new signature would work with 0 varargs. The problem arises when you
> hot swap the geode dependencies for that client app without recompiling, as
> the bytecode no longer matches. Since we do support the hot swap use case
> for CVEs etc, I see now this is considered a breaking API change.
>
> I'll change this to use a method overload instead, it should be a pretty
> simple fix. Luckily, this issue didn't make it into any Geode releases.
>
> Thanks,
> Ryan
>
>
> On Tue, May 14, 2019 at 10:47 AM Udo Kohlmeyer <ud...@apache.org> wrote:
>
> > How did this slip the review process that the signature of a public API
> > was changed?
> >
> > Well done Gester for finding this!!
> >
> > --Udo
> >
> > On 5/14/19 10:40, Dan Smith wrote:
> > > I think the changes for GEODE-6327 broke backwards compatibility in
> > > JSONFormatter with the change from fromJSON(String jsonString) to
> > > fromJSON(String jsonString, String... identityFields)
> > >
> > > Adding an additional varargs parameter to the method breaks code that
> was
> > > compiled against the non-varargs version. I think we need to overload
> the
> > > method instead.
> > >
> > > Thanks to Gester for discovering this with his test!
> > >
> > > -Dan
> > >
> >
>
Re: Backwards compatibility issue with JSONFormatter
Posted by Ryan McMahon <rm...@pivotal.io>.
Hi all,
This is my mistake - it was a misunderstanding of what constitutes a
breaking public API change. If a client app were to recompile using the
new bits with the new method signature, there wouldn't be an issue because
the new signature would work with 0 varargs. The problem arises when you
hot swap the geode dependencies for that client app without recompiling, as
the bytecode no longer matches. Since we do support the hot swap use case
for CVEs etc, I see now this is considered a breaking API change.
I'll change this to use a method overload instead, it should be a pretty
simple fix. Luckily, this issue didn't make it into any Geode releases.
Thanks,
Ryan
On Tue, May 14, 2019 at 10:47 AM Udo Kohlmeyer <ud...@apache.org> wrote:
> How did this slip the review process that the signature of a public API
> was changed?
>
> Well done Gester for finding this!!
>
> --Udo
>
> On 5/14/19 10:40, Dan Smith wrote:
> > I think the changes for GEODE-6327 broke backwards compatibility in
> > JSONFormatter with the change from fromJSON(String jsonString) to
> > fromJSON(String jsonString, String... identityFields)
> >
> > Adding an additional varargs parameter to the method breaks code that was
> > compiled against the non-varargs version. I think we need to overload the
> > method instead.
> >
> > Thanks to Gester for discovering this with his test!
> >
> > -Dan
> >
>
Re: Backwards compatibility issue with JSONFormatter
Posted by Udo Kohlmeyer <ud...@apache.org>.
How did this slip the review process that the signature of a public API
was changed?
Well done Gester for finding this!!
--Udo
On 5/14/19 10:40, Dan Smith wrote:
> I think the changes for GEODE-6327 broke backwards compatibility in
> JSONFormatter with the change from fromJSON(String jsonString) to
> fromJSON(String jsonString, String... identityFields)
>
> Adding an additional varargs parameter to the method breaks code that was
> compiled against the non-varargs version. I think we need to overload the
> method instead.
>
> Thanks to Gester for discovering this with his test!
>
> -Dan
>