Re: [xwiki-devs] [VOTE] REST API Changes not passing CLIRR. Ignore errors

Hi everybody On Wed, Oct 10, 2012 at 4:37 PM, Ludovic Dubost <ludovic(a)xwiki.com> wrote: I did quite a lot of work on this pull request with several changes, mostly related to keep things consistent and some bugfixes. It seems to me that the pull request is ready to be merged. The code is not complex, but since query building and option handling make it quite complicated it would be good if somebody could have a look at it before I merge it. I also wrote some integration tests to check that things are working fine: https://github.com/xwiki/xwiki-enterprise/pull/33 Thanks, Fabio > Ludovic > > 2012/10/8 Ludovic Dubost <ludovic(a)xwiki.com> > >> Hi, >> >> All votes told to move the code to internal packages. >> >> This is something I can do, however I'm not sure exactly which code >> should go into the internal package and which one should stay where it >> is. >> >> Right now here is the list of classes failing or not the CLIRR tests >> >> Not failing: >> >> org.xwiki.rest.ComponentsObjectFactory >> org.xwiki.rest.Constants >> org.xwiki.rest.RangeIterable >> org.xwiki.rest.Relations >> org.xwiki.rest.Utils >> org.xwiki.rest.XWikiAuthentication >> org.xwiki.rest.XWikiJaxRsApplication >> org.xwiki.rest.XWikiResource >> org.xwiki.rest.XWikiRestComponent >> org.xwiki.rest.XWikiRestletJaxRsApplication >> org.xwiki.rest.XWikiRestletServlet >> org.xwiki.rest.XWikiSecretVerifier >> org.xwiki.rest.XWikiSetupCleanupFilter >> org.xwiki.rest.exceptions.QueryExceptionMapper >> org.xwiki.rest.exceptions.XWikiExceptionMapper >> org.xwiki.rest.representations.TextPlainReader >> org.xwiki.rest.representations.comments.FormUrlEncodedCommentReader >> org.xwiki.rest.representations.comments.TextPlainCommentReader >> org.xwiki.rest.representations.objects.FormUrlEncodedObjectReader >> org.xwiki.rest.representations.objects.FormUrlEncodedPropertyReader >> org.xwiki.rest.representations.objects.TextPlainPropertyReader >> org.xwiki.rest.representations.pages.FormUrlEncodedPageReader >> org.xwiki.rest.representations.pages.TextPlainPageReader >> org.xwiki.rest.representations.tags.FormUrlEncodedTagsReader >> org.xwiki.rest.representations.tags.TextPlainTagsReader >> org.xwiki.rest.resources.BrowserAuthenticationResource >> org.xwiki.rest.resources.RootResource >> org.xwiki.rest.resources.SyntaxesResource >> org.xwiki.rest.resources.attachments.AttachmentAtPageVersionResource >> org.xwiki.rest.resources.attachments.AttachmentHistoryResource >> org.xwiki.rest.resources.attachments.AttachmentResource >> org.xwiki.rest.resources.attachments.AttachmentVersionResource >> org.xwiki.rest.resources.classes.ClassPropertiesResource >> org.xwiki.rest.resources.classes.ClassPropertyResource >> org.xwiki.rest.resources.classes.ClassResource >> org.xwiki.rest.resources.classes.ClassesResource >> org.xwiki.rest.resources.objects.BaseObjectsResource >> org.xwiki.rest.resources.pages.ModifiablePageResource >> org.xwiki.rest.resources.pages.PageTagsResource >> org.xwiki.rest.resources.pages.PageTranslationsResource >> org.xwiki.rest.resources.spaces.SpaceResource >> org.xwiki.rest.resources.spaces.SpacesResource >> org.xwiki.rest.resources.tags.TagsResource >> org.xwiki.rest.resources.wikis.WikiPagesResource >> org.xwiki.rest.resources.wikis.WikiSearchQueryResource >> org.xwiki.rest.resources.wikis.WikisResource >> >> Failing: >> >> org.xwiki.rest.DomainObjectFactory >> org.xwiki.rest.resources.BaseAttachmentsResource >> org.xwiki.rest.resources.BaseSearchResult >> org.xwiki.rest.resources.ModificationsResource >> org.xwiki.rest.resources.attachments.AttachmentsAtPageVersionResource >> org.xwiki.rest.resources.attachments.AttachmentsResource >> org.xwiki.rest.resources.comments.CommentResource >> org.xwiki.rest.resources.comments.CommentVersionResource >> org.xwiki.rest.resources.comments.CommentsResource >> org.xwiki.rest.resources.comments.CommentsVersionResource >> org.xwiki.rest.resources.objects.AllObjectsForClassNameResource >> org.xwiki.rest.resources.objects.ObjectAtPageVersionResource >> org.xwiki.rest.resources.objects.ObjectPropertiesAtPageVersionResource >> org.xwiki.rest.resources.objects.ObjectPropertiesResource >> org.xwiki.rest.resources.objects.ObjectPropertyAtPageVersionResource >> org.xwiki.rest.resources.objects.ObjectPropertyResource >> org.xwiki.rest.resources.objects.ObjectResource >> org.xwiki.rest.resources.objects.ObjectsAtPageVersionResource >> org.xwiki.rest.resources.objects.ObjectsForClassNameResource >> org.xwiki.rest.resources.objects.ObjectsResource >> org.xwiki.rest.resources.pages.PageChildrenResource >> org.xwiki.rest.resources.pages.PageHistoryResource >> org.xwiki.rest.resources.pages.PageResource >> org.xwiki.rest.resources.pages.PageTranslationHistoryResource >> org.xwiki.rest.resources.pages.PageTranslationResource >> org.xwiki.rest.resources.pages.PageTranslationVersionResource >> org.xwiki.rest.resources.pages.PageVersionResource >> org.xwiki.rest.resources.pages.PagesResource >> org.xwiki.rest.resources.spaces.SpaceAttachmentsResource >> org.xwiki.rest.resources.spaces.SpaceSearchResource >> org.xwiki.rest.resources.tags.PagesForTagsResource >> org.xwiki.rest.resources.wikis.WikiAttachmentsResource >> org.xwiki.rest.resources.wikis.WikiSearchResource >> >> >> So which one of these are not internal ? >> >> Ludovic >> >> 2012/8/25 Thomas Mortagne <thomas.mortagne(a)xwiki.com>om>: >>> On Tue, Aug 21, 2012 at 10:18 PM, Vincent Massol <vincent(a)massol.net> >> wrote: >>>> >>>> On Aug 21, 2012, at 2:18 PM, Fabio Mancinelli wrote: >>>> >>>>> On Fri, Jul 27, 2012 at 10:12 AM, Ludovic Dubost <ludovic(a)xwiki.com> >> wrote: >>>>>> As part of rest improvements to display pretty names of users and >>>>>> other improvements, I'm getting CLIRR errors because of API changes of >>>>>> the model and of public class: >>>>>> >>>>>> >>>>>> 1/ Model CLIRR error because the version field has been moved to >>>>>> PageSummary from Page. Page extends PageSummary. I need the version >>>>>> field also in representations sending back only PageSummaries. >>>>>> Unfortunately CLIRR does not realize that the version field is still >>>>>> there when moved to the super class. I believe it's safe to ignore >>>>>> this error. Howerver I've put ignore all errors on the Page class as I >>>>>> don't have a way to ignore this specific error >>>>> Yep, I think it's safe. We're adding stuff in a representation (page >>>>> summary) and keeping it also in the other, so API-wise it's ok. >>>> >>>> Note that CLIRR doesn't have false positives so if it complains it >> means there's a breakage. The only decision to take is whether it's an >> "acceptable" breakage, i.e. we voluntarily break assuming that nobody was >> using it and accept that a few users might get broken. >>>> >>>>>> 2/ CLIRR errors because of parameter additions to objects that are >>>>>> used (I think) only internally by the REST server API. Here are the >>>>>> errors: >>>>>> >>>>>> [ERROR] org.xwiki.rest.DomainObjectFactory: In method 'public >>>>>> org.xwiki.rest.model.jaxb.Attachment >>>>>> createAttachment(org.xwiki.rest.model.jaxb.ObjectFactory, >>>>>> java.net.URI, com.xpn.xwiki.api.Attachment, java.lang.String, >>>>>> java.lang.String)' the number of arguments has changed >>>>> >>>>> The DomainObjectFactory is actually a utility class that is used to >>>>> build REST-model objects from XWiki-model objects. >>>>> It has been created just to prevent code duplication in resource >>>>> implementations. >>>> >>>> The question is whether this is supposed to be a SPI or not. >>>> >>>>> Now I think it's unlikely that somebody uses it outside the REST >>>>> module (a quick grep confirmed this for platform). >>>>> >>>>> The only use case for a developer of a module to use this class is if >>>>> she wants to return a REST-model object and build it using the utility >>>>> methods. >>>>> I think this is quite unlikely. >>>>> >>>>> AFAIU all parameters additions are about "pretty names" >>>>> ( >> https://github.com/ldubost/xwiki-platform/compare/master...bd49bcc84e1dec3d… >> ) >>>>> >>>>> If we want to be conservative we might do the following: we can add >>>>> the new methods and preserve the old ones making them call the new >>>>> ones with default parameters. >>>>> >>>>> * false in methods like this >> https://github.com/ldubost/xwiki-platform/compare/master...bd49bcc84e1dec3d… >>>>> * null, false in methods like this >> https://github.com/ldubost/xwiki-platform/compare/master...bd49bcc84e1dec3d… >>>>> . This implies that in the new implementation the if statement should >>>>> also check for null values (like in this case: >> https://github.com/ldubost/xwiki-platform/compare/master...bd49bcc84e1dec3d… >> ) >>>>> >>>>> We could also think about whether continuing to keep this class in the >>>>> public API. It could make sense but I think that nobody will ever use >>>>> it so we can start to @deprecate it and eventually move it in internal >>>>> packages. >>>> >>>> Based on what you say I'd say that these classes/methods were put >> public by mistake and should all be moved to the internal package without >> going through deprecation. >>> >>> +1 for internal, if it's not supposed to be used outside of the module >>> that's the definition of internal >>> >>>> >>>> Of course this means no other other XWiki modules should use them >> either since they're internal. >>>> >>>> Also this means that we don't provide any SPI either since SPI are >> user-public. Is that what we want? >>>> >>>> Thanks >>>> -Vincent >>>> _______________________________________________ >>>> devs mailing list >>>> devs(a)xwiki.org >>>> http://lists.xwiki.org/mailman/listinfo/devs >>> >>> >>> >>> -- >>> Thomas Mortagne >>> _______________________________________________ >>> devs mailing list >>> devs(a)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 > > > > -- > Ludovic Dubost > Founder and CEO > Blog: http://blog.ludovic.org/ > XWiki: http://www.xwiki.com > Skype: ldubost GTalk: ldubost > _______________________________________________ > devs mailing list > devs(a)xwiki.org > http://lists.xwiki.org/mailman/listinfo/devs