[xwiki-devs] API breakage in PropertyClass API during XWiki 4.3M2
Hi Devs, Passing CLIRR is far from sufficient to stay backward compatible or document incompatible changes ! I know that you know that already, but I want you to remind it once more because while I was on the road showcasing a site, I had to discover that a major feature of the site got broken, and it was silently true for a while apparently. I missed some potential leads, which get me really upset, so I waited a while before writing. The cause was a very simple incompatible change introduced in 4.3M2 ! But tracking it down was not easy since this API breakage was not reported in the RN. Here it is: com.xpn.xwiki.objects.classes.PropertyClass#getClassType is no more reporting the full classname of the type, but now drop the "Class" suffix. So, "com.xpn.xwiki.objects.classes.StringClass" become "String", "com.xpn.xwiki.objects.classes.NumberClass" become "Number"... and so on. The unprivileged API com.xpn.xwiki.api.PropertyClass#getClassType() was also impacted since "StringClass" become "String", "NumberClass" become "Number" and so on. The javadoc of the latter function get updated, meaning that we were really conscious a breakage was done here. Even more conscious, the following was applied in the toXML() method: String classType = getClassType(); if (this.getClass().getSimpleName().equals(classType + "Class")) { // Keep exporting the full Java class name for old/default property types to avoid breaking the XAR format // (to allow XClasses created with the current version of XWiki to be imported in an older version). classType = this.getClass().getName(); } The com.xpn.xwiki.objects.meta.MetaClass was also, since it was using those name as property names. Some compatibility change was applied as well, with explicit comments. But apparently, no one notice (including myself !), and this incompatible change goes through without even a mention in the RN, nor in the the jira issue (XWIKI-8355). So guys, I have no intend to blame anyone here, but I couldn't warn you more about potential consequence of incompatible changes. These need to be discuss, or at least clearly mentioned in the RN. Do not rely on CLIRR for doing the job, it don't. Probably adding a mention about those changes in the RN of 4.3 is better now than never, Marius, could you take care of it ? Thanks for your attention, -- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
Hi Denis, Sorry for breaking your code, but in http://lists.xwiki.org/pipermail/devs/2012-October/052487.html you seemed to agree with me that relying on the technical class name is not good. While I preserved the full name in the XAR export, as shown by the code you pasted below, I keep the short, generic form, in the platform code. I'm curious to know how were you using classes.PropertyClass#getClassType() in your code that it got broken. As for api.PropertyClass#getClassType(), again, I don't think relying on the technical class name is good, and, as you can see, api.PropertyClass#getType() (the 'pretty' version of getClassType) didn't change it's behaviour because I really thought that is the method really used in scripts, not getClassType(). This doesn't excuse the lack of mention in the release notes though. I'll fix that. On Sun, Jun 23, 2013 at 1:51 PM, Denis Gervalle <dgl@softec.lu> wrote:
Hi Devs,
Passing CLIRR is far from sufficient to stay backward compatible or document incompatible changes ! I know that you know that already, but I want you to remind it once more because while I was on the road showcasing a site, I had to discover that a major feature of the site got broken, and it was silently true for a while apparently. I missed some potential leads, which get me really upset, so I waited a while before writing.
The cause was a very simple incompatible change introduced in 4.3M2 ! But tracking it down was not easy since this API breakage was not reported in the RN.
Here it is:
com.xpn.xwiki.objects.classes.PropertyClass#getClassType is no more reporting the full classname of the type, but now drop the "Class" suffix. So, "com.xpn.xwiki.objects.classes.StringClass" become "String", "com.xpn.xwiki.objects.classes.NumberClass" become "Number"... and so on.
The unprivileged API com.xpn.xwiki.api.PropertyClass#getClassType() was also impacted since "StringClass" become "String", "NumberClass" become "Number" and so on.
The javadoc of the latter function get updated, meaning that we were really conscious a breakage was done here. Even more conscious, the following was applied in the toXML() method:
String classType = getClassType(); if (this.getClass().getSimpleName().equals(classType + "Class")) { // Keep exporting the full Java class name for old/default property types to avoid breaking the XAR format // (to allow XClasses created with the current version of XWiki to be imported in an older version). classType = this.getClass().getName(); }
The com.xpn.xwiki.objects.meta.MetaClass was also, since it was using those name as property names. Some compatibility change was applied as well, with explicit comments.
But apparently, no one notice (including myself !), and this incompatible change goes through without even a mention in the RN, nor in the the jira issue (XWIKI-8355).
So guys, I have no intend to blame anyone here, but I couldn't warn you more about potential consequence of incompatible changes. These need to be discuss, or at least clearly mentioned in the RN. Do not rely on CLIRR for doing the job, it don't.
Probably adding a mention about those changes in the RN of 4.3 is better now than never, Marius, could you take care of it ?
Thanks for your attention,
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO _______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
Hi Marius, On Tue, Jun 25, 2013 at 9:58 AM, Marius Dumitru Florea < mariusdumitru.florea@xwiki.com> wrote:
Hi Denis,
Sorry for breaking your code, but in
Just to make it clear, that was really not my goal to get your personal apologize here, or to blame you personally. Once again, I just want all of us (including me !) to remember that a minor change can have serious consequences, and that CLIRR is not a tools we can rely on blindly to keep our code compatible. I am sure we are all sorry not to have catched that issue before, because we are all concerned.
http://lists.xwiki.org/pipermail/devs/2012-October/052487.html you seemed to agree with me that relying on the technical class name is not good. While I preserved the full name in the XAR export, as shown by the code you pasted below, I keep the short, generic form, in the platform code. I'm curious to know how were you using classes.PropertyClass#getClassType() in your code that it got broken.
If there is someone to blame, it would be me, for not catching the bad usage we were making of this API, since we I know a better way that could have avoid using it. Anyway, to answer your question, we were using it to know if the type of the properties is a string or a number and choose between a "like" and an "=" in a "where" clause expression.
As for api.PropertyClass#getClassType(), again, I don't think relying on the technical class name is good, and, as you can see, api.PropertyClass#getType() (the 'pretty' version of getClassType) didn't change it's behaviour because I really thought that is the method really used in scripts, not getClassType().
I totally agree with you, and I would have even change the API version as well. Doing it half way is not so good IMO since it increase complexity, and could cause confusion. So we may also consider changing the API version as well, WDYT ?
This doesn't excuse the lack of mention in the release notes though. I'll fix that.
That was really the point, being careful to clearly mention all our incompatible changes in the RN. Personally, I think we are sometime too conservative in that matter, and that properly explained incompatible change does not hurt. Thanks for fixing this and taking care for the future :)
On Sun, Jun 23, 2013 at 1:51 PM, Denis Gervalle <dgl@softec.lu> wrote:
Hi Devs,
Passing CLIRR is far from sufficient to stay backward compatible or document incompatible changes ! I know that you know that already, but I want you to remind it once more because while I was on the road showcasing a site, I had to discover that a major feature of the site got broken, and it was silently true for a while apparently. I missed some potential leads, which get me really upset, so I waited a while before writing.
The cause was a very simple incompatible change introduced in 4.3M2 ! But tracking it down was not easy since this API breakage was not reported in the RN.
Here it is:
com.xpn.xwiki.objects.classes.PropertyClass#getClassType is no more reporting the full classname of the type, but now drop the "Class" suffix. So, "com.xpn.xwiki.objects.classes.StringClass" become "String", "com.xpn.xwiki.objects.classes.NumberClass" become "Number"... and so on.
The unprivileged API com.xpn.xwiki.api.PropertyClass#getClassType() was also impacted since "StringClass" become "String", "NumberClass" become "Number" and so on.
The javadoc of the latter function get updated, meaning that we were really conscious a breakage was done here. Even more conscious, the following was applied in the toXML() method:
String classType = getClassType(); if (this.getClass().getSimpleName().equals(classType + "Class")) { // Keep exporting the full Java class name for old/default property types to avoid breaking the XAR format // (to allow XClasses created with the current version of XWiki to be imported in an older version). classType = this.getClass().getName(); }
The com.xpn.xwiki.objects.meta.MetaClass was also, since it was using those name as property names. Some compatibility change was applied as well, with explicit comments.
But apparently, no one notice (including myself !), and this incompatible change goes through without even a mention in the RN, nor in the the jira issue (XWIKI-8355).
So guys, I have no intend to blame anyone here, but I couldn't warn you more about potential consequence of incompatible changes. These need to be discuss, or at least clearly mentioned in the RN. Do not rely on CLIRR for doing the job, it don't.
Probably adding a mention about those changes in the RN of 4.3 is better now than never, Marius, could you take care of it ?
Thanks for your attention,
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO _______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
This also broker XEclipse's object editor. We are working on a fix with Fabio. Ludovic 2013/6/23 Denis Gervalle <dgl@softec.lu>
Hi Devs,
Passing CLIRR is far from sufficient to stay backward compatible or document incompatible changes ! I know that you know that already, but I want you to remind it once more because while I was on the road showcasing a site, I had to discover that a major feature of the site got broken, and it was silently true for a while apparently. I missed some potential leads, which get me really upset, so I waited a while before writing.
The cause was a very simple incompatible change introduced in 4.3M2 ! But tracking it down was not easy since this API breakage was not reported in the RN.
Here it is:
com.xpn.xwiki.objects.classes.PropertyClass#getClassType is no more reporting the full classname of the type, but now drop the "Class" suffix. So, "com.xpn.xwiki.objects.classes.StringClass" become "String", "com.xpn.xwiki.objects.classes.NumberClass" become "Number"... and so on.
The unprivileged API com.xpn.xwiki.api.PropertyClass#getClassType() was also impacted since "StringClass" become "String", "NumberClass" become "Number" and so on.
The javadoc of the latter function get updated, meaning that we were really conscious a breakage was done here. Even more conscious, the following was applied in the toXML() method:
String classType = getClassType(); if (this.getClass().getSimpleName().equals(classType + "Class")) { // Keep exporting the full Java class name for old/default property types to avoid breaking the XAR format // (to allow XClasses created with the current version of XWiki to be imported in an older version). classType = this.getClass().getName(); }
The com.xpn.xwiki.objects.meta.MetaClass was also, since it was using those name as property names. Some compatibility change was applied as well, with explicit comments.
But apparently, no one notice (including myself !), and this incompatible change goes through without even a mention in the RN, nor in the the jira issue (XWIKI-8355).
So guys, I have no intend to blame anyone here, but I couldn't warn you more about potential consequence of incompatible changes. These need to be discuss, or at least clearly mentioned in the RN. Do not rely on CLIRR for doing the job, it don't.
Probably adding a mention about those changes in the RN of 4.3 is better now than never, Marius, could you take care of it ?
Thanks for your attention,
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO _______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
-- Ludovic Dubost Founder and CEO Blog: http://blog.ludovic.org/ XWiki: http://www.xwiki.com Skype: ldubost GTalk: ldubost
participants (3)
-
Denis Gervalle -
Ludovic Dubost -
Marius Dumitru Florea