API breakage in DocumentAccessBridge

Development
#1

Hi everyone,

I broke the API backward compatibility in XWiki 15.5RC1 by adding a new method to the DocumentAccessBridge#getCurrentDocument() without sending a vote for it. See https://www.xwiki.org/xwiki/bin/view/ReleaseNotes/Data/XWiki/15.5RC1/#HRealbreakages . The question is now how to handle this:

  1. Leave it like this (accept the API breakage because having this new method there makes sense and because chances that someone implemented the DocumentAccessBridge interface without extending our default implementation are low)
  2. Add a default implementation that return null
    @Unstable
default DocumentModelBridge getCurrentDocument()
{
    return null;
}
  3. Add a default implementation that uses getTranslatedDocumentInstance:
    @Unstable
default DocumentModelBridge getCurrentDocument()
{
    DocumentModelBridge result;
    try {
        result = getTranslatedDocumentInstance(getCurrentDocumentReference());
    } catch (Exception e) {
        result = null;
    }
    return result;
}
  4. Remove the new method and use the ExecutionContext instead:
    @Inject
private Execution execution;

execution.getContext().getProperty("xwikicontext").get("doc");
  5. Remove the new method and refactor the code to use oldcore directly (XWikiContext)

On my side:

  1. I find it acceptable and I vote for it.
  2. I don’t like that it creates an inconsistency between getCurrentDocumentReference and getCurrentDocument (normally if one is not null then both should be not null, and if one is null then both should be null)
  3. I don’t like that it gives the false impression that you don’t need to implement this method (it’s optional), but I guess this could be mitigated with a comment explaining the limitations of the default implementation.
  4. I find this too fragile
  5. I don’t like it either because I think it’s better / cleaner to avoid depending on oldcore if possible

So I’m +1 for option 1 but I’ll accept option 3 if others vote for it.

Thanks,
Marius

#2

+1 for 3 or 5. We pride ourselves for not breaking backward compatibility and this is our current rule so -1 for 1 since there are other options that are not costly to implement (unless the options are considered not good by a majority of developers). I’d really not like that we start the dangerous slope of starting to accept breaking backward compat when there are not too complex alternatives as it sets a precedent.

#3

Note that my proposal was to add a comment explaining the limitation of this implementation so that extending developers will see it and know they should provide a better implementation.

#4
  1. +0, I doubt anyone implements DocumentAccessBridge from scratch, so the risk is quite low
  2. -1, this default implementation does not make any sense since it never work
  3. -0, the implementation is a bit misleading since the whole point of getCurrentDocument() is making sure you end up with what’s in the context and not the database one
  4. -1, this is a very fragile way to access the XWikiContext
  5. +0
#5

BTW DocumentAccessBridge was a try to not use oldcore but in practice since we need finalized it and we still haven’t introduced any new model (see XWiki Model 2.0 (Design.XWikiModel20) - XWiki + GitHub - xwiki/xwiki-platform at feature-newmodel) either, it’s not much better than oldcore.

We’ll need to decide:

  • what we do with DAB and the model in general.
  • what work do we put in oldcore to continue removing stuff from it (we stopped doing that and I even believe we’ve started adding stuff to it - not many but a few). If we decide to not do a new model then we might as well rename it to something other than oldcore and remove DAB and co.

That’s a discussion for another thread though :wink:

#6

They might not be costly to implement once but you’re basically asking anyone that will ever have this need in the future (to access the current document instance from the context without depending on old core) to use some hacks / workarounds instead of a component that is meant to serve this need. Doesn’t this add technical debt?

Even when those alternatives are clearly hacks / workarounds?

Which is why i said:

I guess this could be mitigated with a comment explaining the limitations of the default implementation.

I don’t agree. It’s easier to mock and test and it doesn’t bring a ton of dependencies…

#7

I agree that’s the key point here. Ofc we should think about the technical debt and find ways to reduce it. But not break backward compatibility if possible.

BTW I’m not 100% convinced that DAB is the right place to add a method to get the current document from the context. DAB is supposed to replace XWiki and DMB is supposed to replace XWikiDocument. For XWikiContext, the replacement is supposed to be ExecutionContext. We just don’t have a helper ATM to get the current document from the ExecutionContext. I know we introduced getCurrentDocumentReference in DAB (and some others) but maybe that was an error too and code related to the XWikiContext/ExecutionContext should be put elsewhere, in some ContextBridge interface.

At least that would be a way to not break backward compatibility. It would require deprecation/legacy and replacing calls from our code, so not something small.

So far, we’ve avoided changing too much the DAB interface in the past years and we knew it wasn’t complete. That’s why we stopped saying that new modules must not depend on oldcore (something we said at some point but that we dropped quickly since we were lacking lots of bridge APIs and having them all meant doing the same as oldcore…). So if we follow this, the simplest is to depend on oldcore.

It’s not really hard with the test framework we added for testing oldcore stuff. It’s needed anyway as the bridge api is not enough (will never be). I’m reffering to the OldcoreTest annotation.

So for me, we have frozen the bridge api and shouldn’t touch it anymore, until we decide what we do about a new model (hence my slightly out of scope post above about oldcore and the bridge/new model). This is really our technical debt that we need to fix/decide what we do about.

#8

Since there were no additional comments I applied the solution that seem to please both @tmortagne and @vmassol : use old core instead of breaking the API. See Loading... .