geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jared Stewart <stewart.ja...@gmail.com>
Subject Re: Backwards compatibility issue with JSONFormatter
Date Wed, 15 May 2019 01:29:17 GMT
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 <rmcmahon@pivotal.io> wrote:

> Fixed in
>
> https://github.com/apache/geode/commit/cc0b37a504480f554b1884491f44a3cca4113ef5
>
> On Tue, May 14, 2019 at 2:06 PM Ryan McMahon <rmcmahon@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 <abaker@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 <dsmith@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 <rmcmahon@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 <udo@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
> >> >>>>
> >> >>>
> >> >>
> >>
> >>
>

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