Re: [xwiki-devs] BaseObject ID collisions

[Marc Sladek]

Ok, I understand now and agree, thanks for clarifying. Two caveats though:

1) As you mentioned, it applies only to document hash collisions. I'd
however expect object hash collisions to be more frequent because of a
higher object than document count in the average database.

2) It's not guaranteed that all projects using the XWiki platform call
getDocument before saveDocument on the same instance. For example our CMS
'celements' uses an XWikiDocumentCreator

to build XWikiDocuments from scratch without a getDocument call beforehand.
It's possible that there is more code around creating documents this way,
they would not be covered by your proposed change.


On 2 February 2018 at 16:14, Thomas Mortagne 
wrote:

> By "most of the code" I mean most of the code I know :)
>
> AFAIK everything in XWiki platform and in most extensions is doing
> this and it includes of course everything you do trough the standard
> UI.
>
> On Fri, Feb 2, 2018 at 3:51 PM, Marc Sladek 
> wrote:
> > With "most of the code", do you also mean when new documents are being
> > created and stored (the scenario where collisions happen)?
> >
> > Imho there is nothing preventing collisions when doing:
> > * new XWikiDocument(docRef)
> > * modify instance
> > * exists check and save
> >
> > On 2 February 2018 at 15:28, Thomas Mortagne 
> > wrote:
> >
> >> On Fri, Feb 2, 2018 at 3:07 PM, Marc Sladek 
> >> wrote:
> >> > Introducing this certainly doesn't hurt, but 'm not sure how useful it
> >> is.
> >>
> >> I never said it was the solution to all collisions but it will cover
> >> most of the (very rare and never reported) overwrites at 0 cost.
> >>
> >> > Firstly, it can show collisions only after a document is already
> >> > overwritten, thus the damage is already done.
> >>
> >> Again keep in mind that most of the code does:
> >> * getDocument(DocumentReference)
> >> * modify the XWikiDocument instance
> >> * saveDocument()
> >>
> >> so if getDocument() fail you are not going to overwrite anything.
> >>
> >> > Secondly, loadXWikiDoc has to
> >> > be called for the document which doesn't exist anymore, I guess this
> >> > doesn't happen so often since the system won't list it anymore.
> >>
> >> Not sure which use case you are referring to here. Are you talking
> >> about document deleted the a document with a different reference but
> >> same hash is saved ?
> >>
> >> >
> >> > On 2 February 2018 at 13:36, Thomas Mortagne <
> thomas.morta...@xwiki.com>
> >> > wrote:
> >> >
> >> >> For document what could help a lot already without any performance
> >> >> penalty is to compare the loaded document reference and the passed
> one
> >> >> in XWikiHibernateStore#loadXWikiDoc. That's because most of the code
> >> >> in XWiki apply the following logic: getDocument(), modify it,
> >> >> saveDocument().
> >> >>
> >> >> On Fri, Feb 2, 2018 at 12:19 PM, Marc Sladek <
> marc.sla...@synventis.com
> >> >
> >> >> wrote:
> >> >> > Hi Denis,
> >> >> >
> >> >> > Thanks a lot for your answer. I know it's been a while, but I'd
> still
> >> >> like
> >> >> > to follow up on it since it's quite the fundamental issue.
> >> >> >
> >> >> >> Therefore, by improving the hash algorithm, the size of the ids,
> and
> >> the
> >> >> > quality of the hashed key, we have considered ourselves to be saved
> >> >> enough
> >> >> > for a normal usage.
> >> >> >
> >> >> > Still, with enough bad luck, documents and objects may be
> overwritten
> >> >> > without a trace. This is not a stable implementation. And even
> worse,
> >> if
> >> >> on
> >> >> > any XWiki installation hash collisions will happen in the future
> (or
> >> have
> >> >> > already happened since 4.x), they probably won't be easily
> associated
> >> >> with
> >> >> > this issue because it's nearly impossible to debug.
> >> >> >
> >> >> > While I do now understand the motivation to stick with hashes, I'm
> >> still
> >> >> > not sure why a collision detection would be difficult to introduce
> and
> >> >> why
> >> >> > it's even "impossible for some API". Let me briefly outline an
> idea:
> >> >> >
> >> >> > In XWikiHibernateStore#saveXWikiDoc on L615
> >> >> >  >> >> xwiki-platform-core/xwiki-platform-oldcore/src/main/java/
> >> >> com/xpn/xwiki/store/XWikiHibernateStore.java#L615>
> >> >> > an exists check on the doc id is already performed. If now
> >> >> > xwikidoc.fullName is also selected in the HQL, a comparison to
> >> >> > doc.getDocumentReference() can expose an imminent collision before
> >> data
> >> >> is
> >> >> > overwritten. At least an XWikiException should be thrown in this
> >> case. A
> >> >> > similar thing could be done before saving BaseObjects on L1203
> >> >> >  >> >> xwiki-platform-core/xwiki-platform-oldcore/src/main/jav

Re: [xwiki-devs] BaseObject ID collisions

[Thomas Mortagne]

As I said it's not a perfect solution but while waiting for one that
does not reduce a lot the performances it cost nothing and probably
covers most existing use cases.

On Wed, Feb 7, 2018 at 10:55 AM, Marc Sladek  wrote:
> Ok, I understand now and agree, thanks for clarifying. Two caveats though:
>
> 1) As you mentioned, it applies only to document hash collisions. I'd
> however expect object hash collisions to be more frequent because of a
> higher object than document count in the average database.
>
> 2) It's not guaranteed that all projects using the XWiki platform call
> getDocument before saveDocument on the same instance. For example our CMS
> 'celements' uses an XWikiDocumentCreator
> 
> to build XWikiDocuments from scratch without a getDocument call beforehand.
> It's possible that there is more code around creating documents this way,
> they would not be covered by your proposed change.
>
>
> On 2 February 2018 at 16:14, Thomas Mortagne 
> wrote:
>
>> By "most of the code" I mean most of the code I know :)
>>
>> AFAIK everything in XWiki platform and in most extensions is doing
>> this and it includes of course everything you do trough the standard
>> UI.
>>
>> On Fri, Feb 2, 2018 at 3:51 PM, Marc Sladek 
>> wrote:
>> > With "most of the code", do you also mean when new documents are being
>> > created and stored (the scenario where collisions happen)?
>> >
>> > Imho there is nothing preventing collisions when doing:
>> > * new XWikiDocument(docRef)
>> > * modify instance
>> > * exists check and save
>> >
>> > On 2 February 2018 at 15:28, Thomas Mortagne 
>> > wrote:
>> >
>> >> On Fri, Feb 2, 2018 at 3:07 PM, Marc Sladek 
>> >> wrote:
>> >> > Introducing this certainly doesn't hurt, but 'm not sure how useful it
>> >> is.
>> >>
>> >> I never said it was the solution to all collisions but it will cover
>> >> most of the (very rare and never reported) overwrites at 0 cost.
>> >>
>> >> > Firstly, it can show collisions only after a document is already
>> >> > overwritten, thus the damage is already done.
>> >>
>> >> Again keep in mind that most of the code does:
>> >> * getDocument(DocumentReference)
>> >> * modify the XWikiDocument instance
>> >> * saveDocument()
>> >>
>> >> so if getDocument() fail you are not going to overwrite anything.
>> >>
>> >> > Secondly, loadXWikiDoc has to
>> >> > be called for the document which doesn't exist anymore, I guess this
>> >> > doesn't happen so often since the system won't list it anymore.
>> >>
>> >> Not sure which use case you are referring to here. Are you talking
>> >> about document deleted the a document with a different reference but
>> >> same hash is saved ?
>> >>
>> >> >
>> >> > On 2 February 2018 at 13:36, Thomas Mortagne <
>> thomas.morta...@xwiki.com>
>> >> > wrote:
>> >> >
>> >> >> For document what could help a lot already without any performance
>> >> >> penalty is to compare the loaded document reference and the passed
>> one
>> >> >> in XWikiHibernateStore#loadXWikiDoc. That's because most of the code
>> >> >> in XWiki apply the following logic: getDocument(), modify it,
>> >> >> saveDocument().
>> >> >>
>> >> >> On Fri, Feb 2, 2018 at 12:19 PM, Marc Sladek <
>> marc.sla...@synventis.com
>> >> >
>> >> >> wrote:
>> >> >> > Hi Denis,
>> >> >> >
>> >> >> > Thanks a lot for your answer. I know it's been a while, but I'd
>> still
>> >> >> like
>> >> >> > to follow up on it since it's quite the fundamental issue.
>> >> >> >
>> >> >> >> Therefore, by improving the hash algorithm, the size of the ids,
>> and
>> >> the
>> >> >> > quality of the hashed key, we have considered ourselves to be saved
>> >> >> enough
>> >> >> > for a normal usage.
>> >> >> >
>> >> >> > Still, with enough bad luck, documents and objects may be
>> overwritten
>> >> >> > without a trace. This is not a stable implementation. And even
>> worse,
>> >> if
>> >> >> on
>> >> >> > any XWiki installation hash collisions will happen in the future
>> (or
>> >> have
>> >> >> > already happened since 4.x), they probably won't be easily
>> associated
>> >> >> with
>> >> >> > this issue because it's nearly impossible to debug.
>> >> >> >
>> >> >> > While I do now understand the motivation to stick with hashes, I'm
>> >> still
>> >> >> > not sure why a collision detection would be difficult to introduce
>> and
>> >> >> why
>> >> >> > it's even "impossible for some API". Let me briefly outline an
>> idea:
>> >> >> >
>> >> >> > In XWikiHibernateStore#saveXWikiDoc on L615
>> >> >> > > >> >> xwiki-platform-core/xwiki-platform-oldcore/src/main/java/
>> >> >> com/xpn/xwiki/store/XWikiHibernateStore.java#L615>
>> >> >> > an exists check on the doc id is already performed. If now
>> >> >> > xwikidoc.fullName is also selected in the HQL, a comparison to
>> >> >> > doc.getDocumentReference(

[xwiki-devs] [XWiki Day] BFD#165

[Alex Cotiugă]

Hello devs,

This Thursday is BFD#165:
http://dev.xwiki.org/xwiki/bin/view/Community/XWikiDays#HBugfixingdays

Our current status is:
* -95 bugs over 365 days (1 year)
* -67 bugs over 500 days (between 1 and 2 years)
* -281 bugs over 1600 days (4.3 years)
* -685 bugs since the beginning of XWiki

See https://jira.xwiki.org/secure/Dashboard.jspa?selectPageId=10352


Here's the BFD#165 dashboard to follow the progress during the day:
https://jira.xwiki.org/secure/Dashboard.jspa?selectPageId=13949

Happy Bug Fixing Day,
Alex