bug #6730
openDeleting media leads to inconsistent data
90%
Description
The same problem as described in #6527#note-6 also appears in media bulk editor if you try to delete a media that is used for a protolog (TaxonNameDescription). The check only seems to check for TaxonDescription and SpecimenDescription. But there are multiple places where media can be added and reference. We ALWAYS need a general getReferencingObjects check in case any unexpected reference still exists. This is also important for model changes.
Currently if you delete the image and afterwards you open the name the protolog was attached to you get an ObjectNotFoundException exception which pops up in loop (this should also never happen, for NO exception, please feel free to open a new ticket for this).
login : admin
editor version : 4.8.0.201706150749
server : test.e-taxonomy.eu (edit-test) / rem_conf_am
schema version : 4.7.0.0.201710040000
os : Windows Server 2012 R2 6.3 amd64
java : 1.8.0_121
org.eclipse.swt.SWTException: Failed to execute runnable (eu.etaxonomy.taxeditor.remoting.CdmEagerLoadingException: org.hibernate.ObjectNotFoundException: No row with the given identifier exists: [eu.etaxonomy.cdm.model.media.Media#221])
at org.eclipse.swt.SWT.error(SWT.java:4533)
at org.eclipse.swt.SWT.error(SWT.java:4448)
at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:185)
at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4211)
at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3827)
at org.eclipse.jface.window.Window.runEventLoop(Window.java:818)
at org.eclipse.jface.window.Window.open(Window.java:794)
at org.eclipse.jface.dialogs.ErrorDialog.open(ErrorDialog.java:346)
at eu.etaxonomy.taxeditor.model.MessagingUtils$1.run(MessagingUtils.java:279)
at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:182)
at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4211)
at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3827)
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$4.run(PartRenderingEngine.java:1121)
at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1022)
at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:150)
at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:693)
at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:610)
at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:148)
at eu.etaxonomy.taxeditor.Application.start(Application.java:24)
at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:673)
at org.eclipse.equinox.launcher.Main.basicRun(Main.java:610)
at org.eclipse.equinox.launcher.Main.run(Main.java:1519)
Caused by: eu.etaxonomy.taxeditor.remoting.CdmEagerLoadingException: org.hibernate.ObjectNotFoundException: No row with the given identifier exists: [eu.etaxonomy.cdm.model.media.Media#221]
at org.hibernate.collection.internal.AbstractPersistentCollection.remoteInitialize(AbstractPersistentCollection.java:1358)
at org.hibernate.collection.internal.AbstractPersistentCollection.initialize(AbstractPersistentCollection.java:627)
at org.hibernate.collection.internal.AbstractPersistentCollection.read(AbstractPersistentCollection.java:167)
at org.hibernate.collection.internal.AbstractPersistentCollection.readSize(AbstractPersistentCollection.java:184)
at org.hibernate.collection.internal.PersistentList.size(PersistentList.java:114)
at eu.etaxonomy.taxeditor.ui.section.name.ProtologueElement.setEntity(ProtologueElement.java:77)
at eu.etaxonomy.taxeditor.ui.section.name.ProtologueElement.setEntity(ProtologueElement.java:1)
at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionElement.<init>(AbstractEntityCollectionElement.java:106)
at eu.etaxonomy.taxeditor.ui.section.name.ProtologueElement.<init>(ProtologueElement.java:60)
at eu.etaxonomy.taxeditor.ui.element.CdmFormFactory.createEntityCollectionElement(CdmFormFactory.java:2702)
at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.createElementComposite(AbstractEntityCollectionSection.java:263)
at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.createDynamicContents(AbstractEntityCollectionSection.java:251)
at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.renderContent(AbstractEntityCollectionSection.java:220)
at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.internalUpdateSection(AbstractEntityCollectionSection.java:203)
at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.setEntity(AbstractEntityCollectionSection.java:161)
at eu.etaxonomy.taxeditor.ui.section.name.ProtologueSection.setTaxonBase(ProtologueSection.java:105)
at eu.etaxonomy.taxeditor.view.detail.CdmSectionPart.setFormInput(CdmSectionPart.java:91)
at org.eclipse.ui.forms.ManagedForm.setInput(ManagedForm.java:210)
at eu.etaxonomy.taxeditor.view.AbstractCdmDataViewer.refresh(AbstractCdmDataViewer.java:123)
at eu.etaxonomy.taxeditor.view.AbstractCdmDataViewer.setInput(AbstractCdmDataViewer.java:109)
at eu.etaxonomy.taxeditor.view.detail.DetailsViewer.setInput(DetailsViewer.java:161)
at eu.etaxonomy.taxeditor.view.detail.DetailsViewPart.showViewer(DetailsViewPart.java:268)
at eu.etaxonomy.taxeditor.view.detail.DetailsViewPart.selectionChanged_internal(DetailsViewPart.java:136)
at eu.etaxonomy.taxeditor.view.AbstractCdmEditorViewPart$DelaySelection.run(AbstractCdmEditorViewPart.java:52)
at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:182)
... 31 more
Caused by: org.hibernate.ObjectNotFoundException: No row with the given identifier exists: [eu.etaxonomy.cdm.model.media.Media#221]
at org.hibernate.boot.internal.StandardEntityNotFoundDelegate.handleEntityNotFound(StandardEntityNotFoundDelegate.java:28)
at org.hibernate.event.internal.DefaultLoadEventListener.load(DefaultLoadEventListener.java:227)
at org.hibernate.event.internal.DefaultLoadEventListener.proxyOrLoad(DefaultLoadEventListener.java:278)
at org.hibernate.event.internal.DefaultLoadEventListener.doOnLoad(DefaultLoadEventListener.java:121)
at org.hibernate.event.internal.DefaultLoadEventListener.onLoad(DefaultLoadEventListener.java:89)
at org.hibernate.internal.SessionImpl.fireLoad(SessionImpl.java:1129)
at org.hibernate.internal.SessionImpl.internalLoad(SessionImpl.java:1022)
at org.hibernate.type.EntityType.resolveIdentifier(EntityType.java:632)
at org.hibernate.type.EntityType.resolve(EntityType.java:424)
at org.hibernate.type.EntityType.nullSafeGet(EntityType.java:262)
at org.hibernate.persister.collection.AbstractCollectionPersister.readElement(AbstractCollectionPersister.java:833)
at org.hibernate.collection.internal.PersistentList.readFrom(PersistentList.java:382)
at org.hibernate.loader.Loader.readCollectionElement(Loader.java:1353)
at org.hibernate.loader.Loader.readCollectionElements(Loader.java:892)
at org.hibernate.loader.Loader.getRowFromResultSet(Loader.java:738)
at org.hibernate.loader.Loader.processResultSet(Loader.java:972)
at org.hibernate.loader.Loader.doQuery(Loader.java:930)
at org.hibernate.loader.Loader.doQueryAndInitializeNonLazyCollections(Loader.java:336)
at org.hibernate.loader.Loader.doList(Loader.java:2611)
at org.hibernate.loader.Loader.doList(Loader.java:2594)
at org.hibernate.loader.Loader.listIgnoreQueryCache(Loader.java:2423)
at org.hibernate.loader.Loader.list(Loader.java:2418)
at org.hibernate.loader.hql.QueryLoader.list(QueryLoader.java:501)
at org.hibernate.hql.internal.ast.QueryTranslatorImpl.list(QueryTranslatorImpl.java:371)
at org.hibernate.engine.query.spi.HQLQueryPlan.performList(HQLQueryPlan.java:216)
at org.hibernate.internal.SessionImpl.list(SessionImpl.java:1326)
at org.hibernate.internal.QueryImpl.list(QueryImpl.java:87)
at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.bulkLoadLazyCollections(AdvancedBeanInitializer.java:526)
at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.bulkLoadLazies(AdvancedBeanInitializer.java:435)
at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeNodeNoWildcard(AdvancedBeanInitializer.java:244)
at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeNode(AdvancedBeanInitializer.java:125)
at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeNodeRecursive(AdvancedBeanInitializer.java:105)
at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeNodeRecursive(AdvancedBeanInitializer.java:107)
at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeAll(AdvancedBeanInitializer.java:85)
at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initialize(AdvancedBeanInitializer.java:57)
at eu.etaxonomy.cdm.persistence.dao.hibernate.common.CdmEntityDaoBase.load(CdmEntityDaoBase.java:665)
at eu.etaxonomy.cdm.persistence.dao.hibernate.common.CdmGenericDaoImpl.initializeCollection(CdmGenericDaoImpl.java:868)
at eu.etaxonomy.cdm.api.service.CommonServiceImpl.initializeCollection(CommonServiceImpl.java:351)
at sun.reflect.GeneratedMethodAccessor2070.invoke(Unknown Source)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:302)
at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:190)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:157)
at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:99)
at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:281)
at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:96)
at
...
Related issues
Updated by Andreas Müller over 6 years ago
I also do not fully understand why only the 2 explicit usecases "TaxonDescription" and "SpecimenDescription" are handled in the configurator. We either need to have only one general option "if not used elsewhere" or we have to add a 3rd option "if not used at any other place" if we for some reason want to keep these 2 explicit options.
See #6527 for other solutions (general we should try to use the same options throughout the application if somehow possible)
Updated by Katja Luther about 6 years ago
- Target version changed from Release 4.8 to Release 4.10
Updated by Patrick Plitzner about 6 years ago
- Related to bug #6915: Delete does not work in media view for taxon added
Updated by Katja Luther about 6 years ago
- Status changed from New to Resolved
now there are three options for deletion:
- only remove from gallery
- delete if not used somewhere else
- delete from everywhere
and if it is used anywhere else and should only if it not used somewhere else it is only removed from the image gallery.
please review
Updated by Katja Luther about 6 years ago
- Assignee changed from Katja Luther to Andreas Müller
- % Done changed from 0 to 80
Updated by Andreas Müller about 6 years ago
- Status changed from Resolved to Feedback
- Assignee changed from Andreas Müller to Katja Luther
- % Done changed from 80 to 40
The original problem (deleting a media from BULK editor which is e.g. referenced by a TaxonNameDescription via protologue) is not yet solved. It is still possible to delete a media that is referenced.
In the configurator only 1 option was shown ("if not used elsewhere"). I did not check this checkbox. The delete was successful anyway and showed the description (recursive) error message.
Updated by Katja Luther about 6 years ago
Andreas Müller wrote:
The original problem (deleting a media from BULK editor which is e.g. referenced by a TaxonNameDescription via protologue) is not yet solved. It is still possible to delete a media that is referenced.
In the configurator only 1 option was shown ("if not used elsewhere"). I did not check this checkbox. The delete was successful anyway and showed the description (recursive) error message.
The message was wrong because the isDeletable method returned ok, now it returns Abort and the message is correct and the line is not deleted.
Updated by Katja Luther about 6 years ago
- Status changed from Feedback to Resolved
now it should work as aspected.
Updated by Andreas Müller about 6 years ago
- Assignee changed from Katja Luther to Andreas Müller
Updated by Andreas Müller about 6 years ago
The "Delete" dialog now offers "Delete" or "Überspringen" (sorry don't know what is shown in English version). Why not "Cancel"?
Updated by Andreas Müller about 6 years ago
- Status changed from Resolved to Feedback
- Assignee changed from Andreas Müller to Katja Luther
The general issue that inconsistent data is created and media are deleted if they shouldn't is fixed now.
However, the code to delete not only the media but also attached TextData and Descriptions is critical. E.g. when the media is attached to a normal text data which contains text AND media the text data will be deleted together with the text. This is probably not wanted behavior.
the full deletion might be a good idea for image galleries, but not for normal text data. Probably we always need to check this.
Also I do not understand why the delete routine for TaxonDescription is different to the one for SpecimenDescription and NameDescription. Is this only by mistake or is there a reason? If by mistake we should better extract the code into an own method to avoid redundant and error prone code.
Also at the end of the routine only in case of TaxonBase the updatedObject is added to the result. Is this on purpose?
(please pull before changing code as I did some minor cleanup on the code)
Updated by Katja Luther about 6 years ago
- Status changed from Feedback to Resolved
- % Done changed from 60 to 50
Applied in changeset cdmlib|b8489aa95407f47ce7d81c2a308fd357bc6032a6.
Updated by Andreas Müller about 6 years ago
Is this for review for me? All issues fixed?
Updated by Katja Luther about 6 years ago
- Assignee changed from Katja Luther to Andreas Müller
hopefully yes, I moved the handling of the textdata in a separate method, check for text in textdata and added all updated objects to the result.
Updated by Andreas Müller about 6 years ago
- Status changed from Resolved to Feedback
- Assignee changed from Andreas Müller to Katja Luther
I did some further code cleaning (d8f50ab8), please check if you think this is correct.
While doing so I realized that
result.addUpdatedObject(updatedObject);
and all the other "updatedObject" related code is handled after
for (CdmBase ref: references){}
This to me looks strange, as only the final updatedObject will be added. Shouldn't this be done for each updatedObject. Could you check if this needs to be changed?
Updated by Katja Luther about 6 years ago
Andreas Müller wrote:
I did some further code cleaning (d8f50ab8), please check if you think this is correct.
While doing so I realized that
result.addUpdatedObject(updatedObject);and all the other "updatedObject" related code is handled after
yes you are right, I moved this part of code into the for loop
for (CdmBase ref: references){}This to me looks strange, as only the final updatedObject will be added. Shouldn't this be done for each updatedObject. Could you check if this needs to be changed?
Updated by Katja Luther about 6 years ago
- Status changed from Feedback to Resolved
- Assignee changed from Katja Luther to Andreas Müller
Updated by Andreas Müller about 6 years ago
- Status changed from Resolved to Feedback
- Assignee changed from Andreas Müller to Katja Luther
This looks good now.
However, I tried to further simplify/unify the code and to set updatedObject always to null for each loop. See 2 commits. Please review if you think this is correct.
Updated by Andreas Müller about 6 years ago
While doing the refactoring I realized that I don't think we should allow Media being removed from MediaSpecimen. A MediaSpecimen is defined by it's media, therefore removing it should not be allowed. Or have we discussed this differently in the past?
Updated by Katja Luther about 6 years ago
the refactored code looks fine.
you are right a media specimen without media does not make sense. I don't know anymore what we decided in the past. I would add it to the isDeletable and delete method
Updated by Andreas Müller about 6 years ago
By the way there are a couple of other places where Media is used. E.g. DefinedTermBase.getMedia, AmplificationResult.getMedia, Sequence.getContigFile + getPherograms, MediaKey (used whereever keys are usable) and maybe more.
What is the expected behavior (of current code and in general) for these Media. Are they also deleted or do we get a warning that the media can't be deleted? If the later than we definitely should remove MediaSpecimen from the list of deletable media and maybe adapt the configuration text to something like "Delete from taxon and specimen descriptions or if used as protologue for taxon names"
Updated by Andreas Müller about 6 years ago
Sorry if I have even one more issue. Looking at the code I wonder if the service.update(updatedObject) command is really necessary. My understanding of update() is that the object is added(reattached) to the current session. But here, the updated object seems to be always loaded within the current session, so no update (with cascading etc.) is needed. The changes to the updatedObject will always be saved if the session/transaction is committed.
Is there a certain reason why we use update here?
Updated by Katja Luther about 6 years ago
Andreas Müller wrote:
By the way there are a couple of other places where Media is used. E.g. DefinedTermBase.getMedia, AmplificationResult.getMedia, Sequence.getContigFile + getPherograms, MediaKey (used whereever keys are usable) and maybe more.
What is the expected behavior (of current code and in general) for these Media. Are they also deleted or do we get a warning that the media can't be deleted? If the later than we definitely should remove MediaSpecimen from the list of deletable media and maybe adapt the configuration text to something like "Delete from taxon and specimen descriptions or if used as protologue for taxon names"
The code now blocks the deletion (isDeletable returns Abort), may be have to discuss this again.
Updated by Katja Luther over 1 year ago
- Status changed from Feedback to Closed
- % Done changed from 50 to 100
Now only media of descriptions and protogues can be deleted if they are used more then once. The messages are adapted.
So I think we can close this ticket.
Updated by Andreas Müller over 1 year ago
- Status changed from Closed to Feedback
- % Done changed from 100 to 90
Andreas Müller wrote in #note-25:
Sorry if I have even one more issue. Looking at the code I wonder if the service.update(updatedObject) command is really necessary. My understanding of update() is that the object is added(reattached) to the current session. But here, the updated object seems to be always loaded within the current session, so no update (with cascading etc.) is needed. The changes to the updatedObject will always be saved if the session/transaction is committed.
Is there a certain reason why we use update here?
Did you have a look on this, too? The call still seems to exist in line 191
Updated by Katja Luther over 1 year ago
Andreas Müller wrote in #note-28:
Andreas Müller wrote in #note-25:
Sorry if I have even one more issue. Looking at the code I wonder if the service.update(updatedObject) command is really necessary. My understanding of update() is that the object is added(reattached) to the current session. But here, the updated object seems to be always loaded within the current session, so no update (with cascading etc.) is needed. The changes to the updatedObject will always be saved if the session/transaction is committed.
Is there a certain reason why we use update here?
Did you have a look on this, too? The call still seems to exist in line 191
I removed it and everything still works.
Updated by Katja Luther over 1 year ago
- Status changed from Feedback to Resolved
- Assignee changed from Katja Luther to Andreas Müller
I think we can close this ticket now?