Opened 13 years ago
Closed 13 years ago
#379 closed defect (fixed)
Deleting an element can remove the following element.
| Reported by: | justinrowles | Owned by: | mcoudert |
|---|---|---|---|
| Priority: | critical | Milestone: | v2.6.3 |
| Component: | General | Version: | v2.6.1 |
| Keywords: | Cc: |
Description
If an element is removed which is the last of its kind, but not the last in its parent, the geonet:child element created overwrites the following element.
Attachments (2)
Change History (8)
by , 13 years ago
| Attachment: | deleteElementOverwritesElement.diff added |
|---|
comment:1 by , 13 years ago
| Priority: | major → critical |
|---|
comment:2 by , 13 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
(1) sounds good to François and me after a quick talk.
(2) I would suggest to skip this element and to log the error. This case should never happens. Throwing an exception here will lead to loose all changes posted in the editor and to get an error pop up not very explicit.
Please find my new patch attached.
by , 13 years ago
| Attachment: | deleteElementOverwritesElement.patch added |
|---|
comment:3 by , 13 years ago
I think the exception throw version is better.
If you use this new patch, then the "el" variable is dereferenced anyway, so you get a NullPointerException at one of lines 1533, 1545 or 1549. This loses all your changes anyway, and gives you no context for the error.
Whether it *should* happen or not is immaterial! ;) I only changed it because it *did* happen (because of the other bug) and I had to debug from the NPE.
comment:4 by , 13 years ago
The "continue" statement avoid to get a NPE, which is in my opinion the main issue. Concerning the fact to log an error or to throw an exception I would say let's discuss it at the next IRC meeting.
(2) This fix should probably be applied at other places in GeoNetwork, where the "Element not found at ref..." is returned. To be checked.
comment:5 by , 13 years ago
Apologies, I didn't see the 'continue'. You're right, there won't be an NPE.
Personally I'd rather not silently ignore evidence of a mismatch between the front and back ends, as then the user will believe changes have been saved that have in fact been ignored. If it should never happen then there's no reason not to throw an exception. YMMV.
I can't make IRC I'm afraid. No IRC access here. Let's stand on opposite sides of the Channel / La Manche and throw rocks at each other to decide :)
The attached diff makes two changes.
(1) The new geonet:child element in INSERTED at the index from which the old element was deleted. Previously it was SET, which overwrote anything following the element to be deleted.
(2) There was a null pointer exception caused by a log instead of a throw IllegalStateException (which is what is done at similar points), where the code went on to dereference the null element anyway.