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