API breakage on Block to add attributes

Development
1

As discussed in this topic, I want to add attribute-related methods to the Block interface, a public API. I need this API to fix a security vulnerability and thus this needs to be backported to the LTS branch 14.10.x. The new methods are the same as for parameters, just with Object values:

Map<String, Object> getAttributes();
Object getAttribute(String name);
void setAttribute(String name, Object value);
void setAttributes(Map<String, Object> attributes);

It is not possible to provide sensible default implementations for these methods as this would require a map to store the attributes. There are two options:

  1. Provide a default implementation that emulates an empty, read-only attribute map, i.e., throw UnsupportedOperationException for the set-methods and return null/an empty map for the get-methods. You can see this currently in the pull request.
  2. Make this a real API breakage.

Note that in any case, the actual implementation is provided in AbstractBlock which is the base class of all existing implementations of Block, I would expect that any extension that adds its own Block implementation also inherits from it. AbstractBlock is also a public API and its documentation explicitly says “All blocks should extend this class.”. Therefore, regardless of the chosen solution, there should be no breakage in practice.

As the default voting period of 72 hours would end during the weekend, I’m extending the vote to February 6, included.

As this is an API breakage, anyways, here is my +0 for option 1 and +1 for option 2.

2

I would prefer option 3:

  • default implementation for getters (the current one, emulating an empty map of attributes)
  • no implementation for setters, since it’s impossible to implement them. In any case -1 to implement them just to throw an exception indicating that they are not implemented since it does not make any sense, as discussed many times it’s much better to acknowledge that this is an API breakage (and indicate that it’s very unlikely that it will cause a problem since we expect most implementations of Block to extend AbstractBlock)
3

+1 for that option

4

Same as Thomas and Manuel.

5

About this, if this is what we prefer, I think we need to update https://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices#HBackwardCompatibility where it says:

When you need to add a new method to an interface there are 2 solutions to preserve backward compatibility:

  • Create a new interface with the new method and deprecate the old one. This means that code using the old interface must be modified to support the 2 interfaces, and that’s not easy.
  • (Recommended) Use Default Methods (introduced in Java 8).

In view of Thomas’s answer we need to add a sub bullet point to the second bullet point about default methods and explain when it shouldn’t be used and what should be done instead.

6

By the way, it seems to have different reference on various Java subject in https://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices and https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/ (and maybe others https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/).

7

The original idea was the following:

What has happened I think is that more topics got added to https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/ and some of these topics are not about coding styles. The idea of the coding style pages were about the code formatting that you have in your IDEs (spaces, tabs, how many, new lines, etc).

So I think we need to move some content from https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/ to https://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices and since that page is getting large too, to split it into several subpages.

I’ll try to propose something.

8

In the end I decided to not to use Block attributes so I’m not going to perform this change and there will be no breakage. This vote has 3 +1 for option 3 so option 3 passes the vote just in case somebody else needs block attributes at a later point.

Note also that in the meantime I discovered that CompositeBlock allows storing string parameters that are ignored by renderers. It would also be possible to add similar blocks that allow storing custom values without affecting rendering (some code might not ignore them though when, e.g., analyzing the XDOM for certain patterns).