You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "rombert (via GitHub)" <gi...@apache.org> on 2023/05/08 13:33:10 UTC
[GitHub] [sling-org-apache-sling-feature-cpconverter] rombert opened a new pull request, #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
rombert opened a new pull request, #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168
Added failing test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] rombert commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538408119
@kwin - I think it's important to preserve the current behaviour and give consumers a chance to gradually adapt. I think warn + proceed with the conversion is fine, as long as the docview file is still generated. For the record, the content loader will happily process this file, given that the JCR repository has the correct mappings.
I think we can also introduce a switch to the cpconverter that allows opt-in to the stricter behaviours that we introduce, so that once a content-package passes with strict checks they can be enabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] rombert commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538475147
... which is something that was working before. I am not making a technical argument, I am making an argument for not needlessly breaking customer projects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] rombert commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538527123
> So your proposal is to keep generating invalid XMLs unless user opted in to fail in that case?
You can call it that way, yes. You can also formulate it as "we are releasing a minor version of the cp converter that might break existing projects, because of a refactoring". Given that our toolchain worked before, I think this is a drastic move and should be reconsidered.
> I see a need to break here as generating invalid XML (with undeclared prefixes) may easily trigger follow-up issues which are close to impossible to detect then.
I'm not sure I follow. I think we are in one of the two scenarios:
| Namespace present in Initial-Content file | Namespace declared in Repository | Outcome |
| ------------- | ------------- |------------- |
| No | Yes | Installation proceeds, based on the repository namespace mapping |
| No | No | Installation is halted |
What we are doing is changing the outcome of the first scenario. Since no one actually complained about the lenient namespace handling and this only came up after a refactoring I am confident this was not a real problem to solve. That is not to say the refactoring was not valuable, I am very glad that it was done, but we should not introduce such behaviour changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538440839
> as long as the docview file is still generated.
That is impossible as the docview will be invalid (it has to have a proper namespace URL for the prefix). How did the old version of cp2fm behave for that IT? Which docview file has been generated, what was stripped? I don't think this ever was a behaviour which is worth to keep to be honest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538480086
So your proposal is to keep generating invalid XMLs unless user opted in to fail in that case?
For me
> not needlessly breaking customer projects.
is incorrect here. I see a need to break here as generating invalid XML (with undeclared prefixes) may easily trigger follow-up issues which are close to impossible to detect then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] reschke commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538518717
FWIW (not taking a position on the other points): undeclared prefixes *are* fatal in XML with namespace processing active. That said, XML *without* namespace processing would be able to parse these files, but leave the namespace processing to the application invoking the XML processor. For JCR DocView import, namespace-aware parsing is required by the JCR spec.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538470179
The XML you shared is simply invalid. XML mandates that every prefix is declared (https://www.w3.org/TR/xml-names/#ns-using).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538633802
There is no simple solution here except for reverting the https://issues.apache.org/jira/browse/SLING-11421 as the FileVault serializer cannot generate invalid DocView XML. Also I don't have capacity to work on a workaround here for the moment (and also my motivation is limited given
> I see a need to break here as generating invalid XML (with undeclared prefixes) may easily trigger follow-up issues which are close to impossible to detect then.
So I would actually appreciate taking this matter to the mailing list to gather more opinions on it but I am also fine with reverting (but this will obviously reintroduce the old bugs which have been fixed by SLING-11421).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] rombert commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538459954
I pushed this additional test + a revert of SLING-11421 to https://github.com/apache/sling-org-apache-sling-feature-cpconverter/tree/issue/SLING-11865-with-revert . You can see exactly what is being generated by running those tests. The particular file that triggers the problem gets converted to
```xml
<?xml version='1.0' encoding='UTF-8'?>
<jcr:root xmlns="" xmlns:nt="http://www.jcp.org/jcr/nt/1.0" xmlns:xml="http://www.w3.org/XML/1998/namespace" xmlns:oak="http://jackrabbit.apache.org/oak/ns/1.0" xmlns:jcr="http://www.jcp.org/jcr/1.0" xmlns:rep="internal" xmlns:my="http://namespace.com/my" xmlns:mix="http://www.jcp.org/jcr/mix/1.0" xmlns:sling="http://sling.apache.org/jcr/sling/1.0" xmlns:vlt="http://www.day.com/jcr/vault/1.0" jcr:primaryType="nt:unstructured" description="{String}This node has been created by a sling bundle." title="{String}My first node">
<cq:EditConfig jcr:primaryType="cq:EditConfig" description="{String}Some dummy data from sling initial content.">
<cq:listeners jcr:primaryType="cq:EditListenersConfig" description="{String}Some dummy data from sling initial content.">
</cq:listeners>
</cq:EditConfig>
<graniteComponent jcr:primaryType="sling:Folder" jcr:mixinTypes="[granite:Component]">
<granite:data jcr:primaryType="nt:unstructured" description="{String}Some dummy data from sling initial content.">
</granite:data>
<my:subnode jcr:primaryType="nt:unstructured" description="{String}Some dummy data from sling initial content.">
</my:subnode>
</graniteComponent>
</jcr:root>
```
To your point on whether this is a behaviour to keep or not, in Sling we try hard to preserve backwards compatiblity, and that breaking existing projects is a very big change which we should only do for very good reasons. Being too restrictive with what we accept as input will only hurt adoption of new versions of CP Converter, leaving deployments on old versions because upgrading was too inconvenient.
Again, this is a conversion that worked on 1.3.0 and not on 1.3.2 and that was installable using the JCR ContentLoader.
If you do believe that this is something that must be eventually enforced, let's please start with a warning and make the behaviour opt-in via a command line flag, so consumers can control the evolution of their project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] rombert commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538638161
> There is no simple solution here except for reverting the https://issues.apache.org/jira/browse/SLING-11421 as the FileVault serializer cannot generate invalid DocView XML. Also I don't have capacity to work on a workaround here for the moment (and also my motivation is limited given
>
> > I see a need to break here as generating invalid XML (with undeclared prefixes) may easily trigger follow-up issues which are close to impossible to detect then.
>
> So I would actually appreciate taking this matter to the mailing list to gather more opinions on it but I am also fine with reverting (but this will obviously reintroduce the old bugs which have been fixed by [SLING-11421](https://issues.apache.org/jira/browse/SLING-11421)).
I think reverting would be a shame. I'll take this to the mailing list, thanks for the clarification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on pull request #168: SLING-11865 - Conversion fails when initial content document does not include namespace declaration
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #168:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/168#issuecomment-1538394880
IMHO throwing an exception for undeclared namespace prefixes is reasonable here (although in the past this might have been silently ignored). @rombert Do you think that this should just be logged and conversion should continue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org