|
Couldn't this issue be handled more gracefully by postponing the space-splitting of the attribute value to when we have mapped the attribute name to a matching property definition? If the defined property is multivalued, then we'd split the attribute value and decode the _xFFFF_ endodings. Otherwise we'd just use the value as-is. Using a special marker character is a bit kludgy.
the multi-value attribute is one of the differentiators being use to identify applicable property definitions.
nt:unstructured for example defines 2 residual properties that only differ in the multi-value attribute. your approach would not be able to unambiguously determine the 'correct' definition because the 'multi-valued' information is lost in document view . btw: i just realized that some related test-cases fail after my changes. i'll have to find out if they do so legitimatly or whether they have to be adapted. Stefan:
> the multi-value attribute is one of the differentiators being use to identify applicable property definitions. > nt:unstructured for example defines 2 residual properties that only differ in the multi-value attribute. > your approach would not be able to unambiguously determine the 'correct' definition because the > 'multi-valued' information is lost in document view . You're right, but that's something I'd expect. If the property name matches both a single- and a multi-valued property and the property value doesn't hint the correct interpretation (like when a reference property contains a space), then it should be reasonable to default for the single-valued definition. We could even have some heuristics about the property value containing _xFFFF_ escapes when the correct property definition cannot otherwise be determined. This approach would cover the original case reported by Tobias as well as a majority of normal use cases. The solution with a marker character only works when doing import/export between Jackrabbit repositories, but fails when importing content from other sources like original XML documents, exports from other JCR implementations, or results of XSL transformations written based on the JCR spec. This solution also explicitly breaks the rules in section 6.4.2.5 of the JSR 170 spec. I'd use the following heuristic for interpreting the import of a document view property named N with value V: 1) If N matches just a single property definition, interpret V according to that definition. 2) Otherwise N matches a single-valued definition S and a multi-valued definition M: 2.1) If V contains whitespace and S is not a string property definition, interpret V according to M 2.2) If V contains _xFFFF_ escapes and M is not a string property definition, interpret V according to S 2.2) If V contains _xFFFF_ escapes, interpret V according to M 2.3) Otherwise interpret V according to S This heuristic doesn't match all cases, but should work pretty much as expected for a majority of use cases. Oops, I hade the sequence numbering wrong (added 2.2 after a bit of thought). The correct sequence is:
1) If N matches just a single property definition, interpret V according to that definition. 2) Otherwise N matches a single-valued definition S and a multi-valued definition M: 2.1) If V contains whitespace and S is not a string property definition, interpret V according to M 2.2) If V contains _xFFFF_ escapes and M is not a string property definition, interpret V according to S 2.3) If V contains _xFFFF_ escapes, interpret V according to M 2.4) Otherwise interpret V according to S Jukka:
> This solution also explicitly breaks the rules in section 6.4.2.5 of the JSR 170 spec. you're right, i wasn't aware of that. btw the previous implementation (pre jcr-325 fix) was neither spec-compliant. until we've got a proper and spec-compliant solution for multi-value handling in document view i changed the current implementation to skip multi-valued properties entirely on document view export as this is explicitly allowed by the spec (svn r384850). document view serialization is per definition 'lossy' and not guaruanteed to roundtrip.
changing type to 'Improvement' as current implementation (skipping mutli-valued properties on document view export) is compliant with "6.4.2.5 Multi-value Properties" of the spec. Assigned to me. I'm working on implementing the heuristics described above.
I've gone through the importer code and come up with a plan on how to implement the interpretation described above. The current importer design makes it quite hard to postpone the value parsing decisions until the applicable property definitions are known, which effectively prevents form implementing the proposed heuristics. To make this easier and to simplify overall code I'd like to encapsulate the value parsing rules to the PropInfo instances so that the SessionImporter or WorkspaceImporter classes wouldn't need to know anything about where the values came from (sys view or doc view) and how they should be parsed. I could then create separate SysViewPropInfo and DocViewPropInfo subclasses to encapsulate the different value parsing rules of the system view and document view XML encodings. Doing this however requires quite extensive refactoring of the current code, so I'd like to get a confirmation on this plan before I proceed.
The attached patch represents the first steps of this refactoring. Unless I've made a mistake, the patch doesn't yet change any of the existing behaviour (except for the location where some exceptions and log messages are generated), it just refactors the code structure so that it is easier to implement the SysViewPropInfo and DocViewPropInfo classes. The reason for publishing the patch at this point is to separate the structural changes from the behavioural changes and thus make the changes easier to review. I will continue with the behavioural changes if this structural change is approved. Below is a breakdown of the refactoring steps included in this patch. I did almost all of these changes using the automatic refactoring tools in Eclipse to minimize the chance of accidentally introducing errors in the code. The patched source also passes all unit tests and seems to import both system view and document view files just as before, which makes me quite confident in the quality of the refactoring. 1) Move the following internal classes and interfaces to new files to make the class structure easier to manage: * Importer.TextValue -> TextValue * Importer.PropInfo -> PropInfo * Importer.NodeInfo -> NodeInfo * TargetImportHandler.AppendableValue -> AppendableValue * TargetImportHandler.BufferedStringValue -> BufferedStringValue * TargetImportHandler.StringValue -> StringValue 2) Remove the NodeInfo and PropInfo setters and make the member fields "private final" to enforce their immutability 3) Refactor the TargetImportHandler.disposePropertyValues(PropInfo) method into PropInfo.dispose() to improve encapsulation 4) Move the AppendableValue.dispose() method up to TextValue and implement it as a null method in StringValue to avoid type casts in PropInfo.dispose() 5) Remove the AppendableValue interface to make the class structure more simple as the SysViewImportHandler can just as well use the BufferedStringValue class directly 6) Add a TextValue.getNamespaceContext() method and corresponding "private final NamespaceResolver nsContext;" fields in StringValue and BufferedStringValue to associate the value instances with the namespace context in which they should be parsed. Remove the "nsContext" argument from Importer.startNode() and use the TextValue.getNamespaceContext() method in SessionImporter and WorkspaceImporter to access the correct namespace context. 7) Refactor the contents of the property iterator loop in WorkspaceImporter.startNode() into PropInfo.apply(NodeState, BatchedItemOperations, NodeTypeRegistry, ReferenceChangeTracker) and the contents of the similar loop in SessionImporter.startNode() into PropInfo.apply(NodeImpl, NamespaceResolver, ReferenceChangeTracker) to simplify the huge startNode() methods and to encapsulate the value parsing logic into PropInfo. This also allows the methods to easily share code through extracted getTargetType() and getApplicablePropertyDef() methods and enables other structural simplifications. 8) Remove the now unneeded PropInfo getters. 9) Refactor the contents of the value iterator loops in the PropInfo.apply() methods to TextValue.getValue(int type, NamespaceResolver resolver) and TextValue.getInternalValue(int type) methods and copy the implementation to StringValue and BufferedStringValue to increase encapsulation. In both cases the implementation can be simplified thanks to local access to the value details. 10) Remove the length(), retrieve(), reader(), and getNamespaceContext() methods from the TextValue interface to ensure proper encapsulation. Those methods can also be fully removed from StringValue, but in BufferedStringValue they still have a purpose due to the more complex storage model. Refactor the JCR_MIXINTYPES branch in DocViewImportContentHandler to use TextValue.getValue() instead of StringValue.retrieve() to get the mixin type names. Overall these structural changes improve a number of quality metrics. The encapsulation level is higher, the method and class bodies are shorter, and even the total amount of code is slightly lower. i'm not sure regarding preserving the current (i.e. local, scoped) namespace context
from the xml document within the StringValue & BufferedStringValue instances. in your patch you just pass a reference to the ns context in the constructor. the ns context however is mutable and could, theoretically at least, change after a *StringValue has been created and before getValue has been called. there should at least be a warning that the values have to be processed during the Importer.startNode call. ohterwise the ns context should be cloned. that however would significantly increase memory usage compared to the current approach. apart from this the patch looks good to me. > in your patch you just pass a reference to the ns context in the constructor.
> the ns context however is mutable and could, theoretically at least, change > after a *StringValue has been created and before getValue has been called. Yes, I tought of this issue as well. The use of the ns context in the patch is temporally identical to the current code in svn, as the *StringValue instances are no longer used after the Importer.startNode() call. This temporal binding is however much less obvious in the patch, so you are right in that there is a threat of further changes breaking the inherent assumption of the ns context not changing during the lifetime of a *StringValue instance. This issue could be quite cleanly solved by explicitly managing the ns context as a stack of immutable prefix mappings instead of using the mutable SAX NamespaceSupport class. A namespace context would be an object with a reference to the parent namespace context and a collection of prefix mappings declared for a given XML element. Namespace resolution would then follow the parent references until a match is found. (This is actually what the NamespaceSupport class does internally, although it only presents a mutable facade that hides this data structure behind the push/pop methods.) I'll make a new version of the patch with modified namespace context handling. Attached a patch that contains the new namespace context implementation. The namespace-context.patch was made against the current SVN trunk and does not contain any of the other refactoring changes so it should be relatively easy to review.
This change contains a lot more manual changes than the xml-refactoring.patch that was mostly tool-generated. I'm however again quite confident about the code quality, it passes all the unit tests and has no trouble handling convoluted XML documents like this: <foo xmlns="foo-ns"><bar xmlns="bar-ns"/></foo> The patch lets the ImportHandler class keep track of the startPrefixMapping calls per each XML element. These local prefix mappings are reported to the target handler using startNamespaceContext() before the related startElement() call. Similarly each endElement() call is followed by a endNamespaceContext() call. The TargetImportHandler class uses these namespace context calls to manage the local nsContext variable so that it always contains a reference to the immutable namespace context of the current XML element. Jukka:
> Attached a patch that contains the new namespace context implementation. +1, looks good. thanks, jukka. I am faced with similar issue...
with CND definition ( @see http://wiki.apache.org/jackrabbit/JackRabbitOnWeblogic ) ... - flix:GENRE (string) mandatory multiple < 'Action', 'Comedy', 'Drama', 'Horror', 'Romance', 'Sci-Fi & Fantasy', 'Thrillers' and Java code nodeTmp .setProperty("flix:GENRE", new String[]{"Action", "Comedy"}); Jackrabbit-repo gets the data like ... /Flix Media Auto/Superbad/flix:GENRE[0] ="Comedy" /Flix Media Auto/Superbad/flix:GENRE[1] ="Action" what is confirmed with the Junit assertion assertEquals(retval.getNode("Superbad").getProperty( "flix:GENRE").getValues().length, 2); But the next line session.exportDocumentView(absPath, System.out, false, false); prints only --System.out <?xml version="1.0" encoding="UTF-8"?> <Flix_x0020_Media_x0020_Auto xmlns:nt="http://www.jcp.org/jcr/nt/1.0" xmlns:ocm="http://jackrabbit.apache.org/ocm" xmlns:rep="internal" xmlns:oxseed="http://jcr.oxseed.com/jcr/1.0/oxseed" xmlns:sv="http://www.jcp.org/jcr/sv/1.0" xmlns:mix="http://www.jcp.org/jcr/mix/1.0" xmlns:fn="http://www.w3.org/2005/xpath-functions" xmlns:jcr="http://www.jcp.org/jcr/1.0" xmlns:flix="http://wlp.bea.com/jcr/1.0/flix" xmlns:fn_old="http://www.w3.org/2004/10/xpath-functions" xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:test="http://www.apache.org/jackrabbit/test" jcr:primaryType="nt:unstructured"> <Superbad jcr:primaryType="flix:MEDIA" flix:CAST="Jonah Hill" flix:DIRECTOR="Greg Mottola" flix:IMDB_LINK="http://www.imdb.com/title/tt0829482/" flix:LENGTH="114" flix:MEDIA_TYPE="DVD" flix:QUOTES="Same-sies!" flix:RELEASE_DATE="2007-09-17T19:33:33.177+02:00" flix:STOCK_STATUS="unreleased" flix:SYNOPSIS="Hilarous!" flix:TITLE="Superbad"/> </Flix_x0020_Media_x0020_Auto> In the same case the SystemViewExport works fine. ... <sv:property sv:name="flix:GENRE" sv:type="String"> <sv:value>Action</sv:value> <sv:value>Comedy</sv:value> </sv:property> ... Is it possible somehow to get DocViewExport for my multi-valued property? As this is compliant with the spec and implementation is not trivial, this bug hasn't been solved yet. Generally there is no garantee that you can map a JCR structure to an XML document view - especially when you use a JCR-only feature such as multi-value properties. See also the comment above:
> changing type to 'Improvement' as current implementation (skipping mutli-valued properties on document view export) is compliant with "6.4.2.5 Multi-value Properties" of the spec. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Reopened
Major
docview roundtripping does not work with multivalue non-string properties
as some of the information, such as property type and multi-value flag, is lost.
the only failsafe way of handling multi-valued properties is to skip them
on export (see "6.4.2.5 Multi-value Properties", jsr-170 specification).
this is probably not desirable from a user perspective.
the following pragmatic approach has been chosen to workaround this issue:
document view export:
------------------------------
- multi-value properties are exported space-separated, with a leading line-feed (
)
as a 'multi-valued'-hint; spaces within a value are encoded as _x0020_;
e.g. ["john doe","donald duck"] => "
john_x0020_doe donald_x0020_duck"
- single-value properties are exported without encoding contained spaces as _x0020_
e.g. "Hello, World!" => "Hello, World!"
document view import:
------------------------------
- attribute values starting with 
 (line-feed) are assumed to represent multiple values,
delimited by spaces; _x0020_ within values are decoded to spaces
e.g. "
john_x0020_doe donald_x0020_duck" => ["john doe","donald duck"]
- all other attribute values are considered to be single values; no space decoding
is performed.
e.g. "Hello, World!" => "Hello, World!"
fixed in svn r384197