While working on required rights, I noticed that the hidden status of a page is always updated when the page is updated via the REST API. This means that when you don’t specify the hidden status, it is implicitly set to false. This even concerns the API that uses an application/x-www-form-urlencoded body. I’ve created XWIKI-22336 for this issue. On the other hand, when just specifying the hidden status in a page update request in the REST API, the page isn’t saved, this is XWIKI-22337.
In order to fix this, we need to be able to distinguish between three values of the hidden field: true, false and not set. Currently, the org.xwiki.rest.model.jaxb.Page.hidden field that is used to represent the received data is of type boolean so it cannot represent that “not set” state. I propose to change this field to Boolean such that we can properly distinguish between the three values. This results in the following breakages:
[ERROR] java.field.typeChanged: field org.xwiki.rest.model.jaxb.Page.hidden: The type of the field changed from 'boolean' to 'java.lang.Boolean'. https://revapi.org/revapi-java/differences.html#java.field.typeChanged
[ERROR] java.annotation.removed: field org.xwiki.rest.model.jaxb.Page.hidden: Element no longer annotated with 'javax.xml.bind.annotation.XmlElement'. https://revapi.org/revapi-java/differences.html#java.annotation.removed
[ERROR] java.method.returnTypeChanged: method java.lang.Boolean org.xwiki.rest.model.jaxb.Page::isHidden(): The return type changed from 'boolean' to 'java.lang.Boolean'. https://revapi.org/revapi-java/differences.html#java.method.returnTypeChanged
[ERROR] java.method.parameterTypeChanged: parameter void org.xwiki.rest.model.jaxb.Page::setHidden(===java.lang.Boolean===): The type of the parameter changed from 'boolean' to 'java.lang.Boolean'. https://revapi.org/revapi-java/differences.html#java.method.parameterTypeChanged
[ERROR] java.method.parameterTypeChanged: parameter org.xwiki.rest.model.jaxb.Page org.xwiki.rest.model.jaxb.Page::withHidden(===java.lang.Boolean===): The type of the parameter changed from 'boolean' to 'java.lang.Boolean'. https://revapi.org/revapi-java/differences.html#java.method.parameterTypeChanged
This vote is about accepting this breakage. I thought this breakage was acceptable because this Page object isn’t supposed to be used outside the REST API. However, I then noticed that it is in fact used in REST clients. Thanks to autoboxing and unboxing, no actual code changes are necessary but if a code is compiled against one version of the Page object and uses the hidden field, it won’t work with the updated version. I’m thus not sure anymore if the breakage is really acceptable.
I’m wondering if we shouldn’t entirely change the behaviour of ModelFactory#toDocument: right now it considers that any provided field leads to a new save, even if the value are exactly the same. So doing a PUT of the page with same title, will lead to a save. We could imagine a different mode, where we only save when there’s actual a change in the data.
I’m realizing by writing this that I’m just moving the problem elsewhere: it’s also a breaking change, not in the API but in the behaviour this time.
So I think I’m +0 for your changes provided that they’ll go only in 16.6+, a REST client code recompiled against the new API would still work with the old one so the breakage is not that bad I think.
The problem is also that it isn’t solving the main problem: we still wouldn’t have any way to distinguish between a request that didn’t provide the hidden field and a request that explicitly set it to false.
Yes, as indicated in the PR, due to the breakage, I don’t suggest to backport this change. Also, this problem exists since a long time, I only found it by studying the code, so I don’t think there is any need to fix this in a specific version. Further, there are easy workarounds if you know the problem, you can just always also specify another field when you want to change the hidden status and when you want to update a hidden page you just also need to specify the hidden field.
Yes, but the problem is that this Page object represents the request, this is all we see of the request. And if the hidden field of that Page object is false, this could either mean that there was no hidden field in the request or that the hidden field was set to false in the request. And from what I can see it is impossible to distinguish between these two states.
Yes, this API is supposed to be used by both servers and clients (which is why it’s not internal) which are based on all sort of tech and not only Java (but I doubt it really happens in practice) so that makes it an important breakage, and, I’m afraid, not really an acceptable one. In theory, the way to go in cases like these would be to deprecate the current API and introduce a new one, one which is based on Boolean. It would be easy in pure Java, but the code generation aspect might make things a bit harder, as I’m not sure how to make the deprecated API call the other one instead of having its own field (but I never really searched for this yet). It’s also hard to find a good name for the new API…
If this was a pure Java class, I would simply change the attribute type, add a setter with a different parameter type and add a method getHidden() that returns Boolean (vs. isHidden() that returns boolean). Not the cleanest solution but it would work. The alternative would be to have a separate flag if the hidden field has ever been set. Unfortunately, I have no idea how this work with the generated code.