Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 358370 - ElementImpl.getAttributeNode() returns a new attribute without a reference to the owning element
Summary: ElementImpl.getAttributeNode() returns a new attribute without a reference to...
Status: RESOLVED FIXED
Alias: None
Product: WTP Source Editing
Classification: WebTools
Component: wst.xml (show other bugs)
Version: 3.3.1   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 3.3.2   Edit
Assignee: Nick Sandonato CLA
QA Contact: Nitin Dahyabhai CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-21 04:26 EDT by Gregor Latuske CLA
Modified: 2011-10-25 16:56 EDT (History)
1 user (show)

See Also:


Attachments
Patch for the trunk. (854 bytes, patch)
2011-09-21 04:28 EDT, Gregor Latuske CLA
no flags Details | Diff
patch (2.42 KB, patch)
2011-09-22 14:05 EDT, Nick Sandonato CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gregor Latuske CLA 2011-09-21 04:26:32 EDT
Build Identifier: 20110615-0604

If no attribute found but a default value exists, the ElementImpl will create a new attribute. This attribute does not have a reference to the owning element, that has just created the attribute.

String implied = getDefaultValue(name, null);
if (implied != null) {
   Attr createdAttribute = getOwnerDocument().createAttribute(name);
   createdAttribute.setNodeValue(implied);
   return createdAttribute;  <-- No owner element set
}

Reproducible: Always

Steps to Reproduce:
1. Load a XML file via the org.eclipse.wst.xml.core DOM implementation (with XML schema).
2. Call ElementImpl.getAttributeNode() with an attribute name that does not exists in the XML file but is part of the schema.
3. The created attribute has no owner element reference (owner element == null).
Comment 1 Gregor Latuske CLA 2011-09-21 04:28:11 EDT
Created attachment 203735 [details]
Patch for the trunk.

This patch sets the owner element for the created attribute.
Comment 2 Nick Sandonato CLA 2011-09-22 14:05:00 EDT
Created attachment 203853 [details]
patch

Gregor,

Thank you very much for your patch! They're always appreciated. Unfortunately, a problem results from it where the actual structured document backing the model ends up with the attribute in the text. This can also cause some firing of notifications that an attribute node has been added, when in reality it has not been.

I've checked in the change to set the ownerElement plus added a unit test.

Thanks again.
Comment 3 Nick Sandonato CLA 2011-09-22 14:08:41 EDT
Changes checked in.
Comment 4 Walter Brunauer CLA 2011-10-25 03:43:42 EDT
I do not consider this approach to be correct. Not the fix of this bug, but the functionality as a whole. I suspect an invalid shortcut taken by the implementer of this functionality. Just a quick thought:

If an attribute physically is not there in the model, but you provide one in case a default value is specified in the schema by creating it on the fly, how could any client code subsequently ever distinguish if the attribute was there before or not? Besides, this ends up in unexpected model changed notifications on querying information only - I would never expect such notifications if I only ask for an attribute. And, I assume it could happen, that this phantom attribute makes it into persistence in case the model is saved. This definitely is also not desired behavior.

All this caused a lot of problems in our application caused by the recent change in the semantics of this method, so we finally worked around it by currently not using any API using this method!

If at all possible, please reconsider your approach. Note, that there is a dedicated API to query for the default value in the schema after all. If somebody needs the default value of an attribute, he explicitly could query for it, independent if the attribute exists or not.
Comment 5 Nick Sandonato CLA 2011-10-25 16:56:22 EDT
(In reply to comment #4)
> I do not consider this approach to be correct. Not the fix of this bug, but the
> functionality as a whole. I suspect an invalid shortcut taken by the
> implementer of this functionality. Just a quick thought:
> 
> If an attribute physically is not there in the model, but you provide one in
> case a default value is specified in the schema by creating it on the fly, how
> could any client code subsequently ever distinguish if the attribute was there
> before or not? Besides, this ends up in unexpected model changed notifications
> on querying information only - I would never expect such notifications if I
> only ask for an attribute. And, I assume it could happen, that this phantom
> attribute makes it into persistence in case the model is saved. This definitely
> is also not desired behavior.
> 
> All this caused a lot of problems in our application caused by the recent
> change in the semantics of this method, so we finally worked around it by
> currently not using any API using this method!
> 
> If at all possible, please reconsider your approach. Note, that there is a
> dedicated API to query for the default value in the schema after all. If
> somebody needs the default value of an attribute, he explicitly could query for
> it, independent if the attribute exists or not.

Hi Walter,

It looks like you're right. It seems like the specification does not call for returning an Attribute with the default value from getAttributeNode in the absence of an existing attribute. I've opened up Bug 362006 to handle this.