anchela commented on code in PR #41:
URL:
https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/41#discussion_r1049613439
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/NodeVisitor.java:
##########
@@ -57,18 +59,33 @@ public void visitCreatePath(CreatePath cp) {
String parentPath = parentPathBuilder.toString();
final String fullPath = String.format("%s/%s", parentPath,
psd.getSegment());
try {
- if (session.itemExists(fullPath)) {
- log.info("Path already exists, nothing to do (and not
checking its primary type for now): {}", fullPath);
- } else {
+ final Node node;
+ if (session.nodeExists(fullPath)) {
+ log.info("Node at {} already exists, checking/adjusting
its types", fullPath);
+ node = session.getNode(fullPath);
+ if (psd.getPrimaryType() != null &&
!node.getPrimaryNodeType().getName().equals(psd.getPrimaryType())) {
+ log.info("Adjusting primary type of node {} to {}",
fullPath, psd.getPrimaryType());
+ node.setPrimaryType(psd.getPrimaryType());
Review Comment:
IMHO this is a fundamental change to the way repo-init deals with the
'create path' statement and i am not entirely sure if it wouldn't be better to
use a dedicated syntax for enforcing the primary type.
personally, i would prefer if the primary type would have been set/adjusted
from the very first version of repoinit. but i fear that changing it now will
cause regressions as consumers of repoinit may have flawed type information
with create-path statements that just work because a given node already
exists.... changing the behavior now would result in these mistakes to become
visible and thus failing the repository start up.
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/NodeVisitor.java:
##########
@@ -57,18 +59,33 @@ public void visitCreatePath(CreatePath cp) {
String parentPath = parentPathBuilder.toString();
final String fullPath = String.format("%s/%s", parentPath,
psd.getSegment());
try {
- if (session.itemExists(fullPath)) {
- log.info("Path already exists, nothing to do (and not
checking its primary type for now): {}", fullPath);
- } else {
+ final Node node;
+ if (session.nodeExists(fullPath)) {
+ log.info("Node at {} already exists, checking/adjusting
its types", fullPath);
+ node = session.getNode(fullPath);
+ if (psd.getPrimaryType() != null &&
!node.getPrimaryNodeType().getName().equals(psd.getPrimaryType())) {
+ log.info("Adjusting primary type of node {} to {}",
fullPath, psd.getPrimaryType());
+ node.setPrimaryType(psd.getPrimaryType());
+ }
+ } else if (!session.propertyExists(fullPath)) {
final Node parent = parentPath.equals("") ?
session.getRootNode() : session.getNode(parentPath);
log.info("Creating node {} with primary type {}",
fullPath, psd.getPrimaryType());
- Node node = addChildNode(parent, psd);
- List<String> mixins = psd.getMixins();
- if (mixins != null) {
- log.info("Adding mixins {} to node {}", mixins,
fullPath);
- for (String mixin : mixins) {
- node.addMixin(mixin);
- }
+ node = addChildNode(parent, psd);
+
+ } else {
+ throw new RepoInitException("There is a property with the
name of the to be created node already at " + fullPath + ", therefore bailing
out here as potentially not supported by the underlying JCR");
Review Comment:
i don't think repo-init should check this. JCR specification leaves it to
the implementation if same-name property/node are allowed and IMHO repo-init
should delegate that verification.
i might be mistaken but i vaguely remember that oak actually supports it.
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/NodeVisitor.java:
##########
@@ -57,18 +59,33 @@ public void visitCreatePath(CreatePath cp) {
String parentPath = parentPathBuilder.toString();
final String fullPath = String.format("%s/%s", parentPath,
psd.getSegment());
try {
- if (session.itemExists(fullPath)) {
- log.info("Path already exists, nothing to do (and not
checking its primary type for now): {}", fullPath);
- } else {
+ final Node node;
+ if (session.nodeExists(fullPath)) {
+ log.info("Node at {} already exists, checking/adjusting
its types", fullPath);
+ node = session.getNode(fullPath);
+ if (psd.getPrimaryType() != null &&
!node.getPrimaryNodeType().getName().equals(psd.getPrimaryType())) {
+ log.info("Adjusting primary type of node {} to {}",
fullPath, psd.getPrimaryType());
+ node.setPrimaryType(psd.getPrimaryType());
+ }
+ } else if (!session.propertyExists(fullPath)) {
final Node parent = parentPath.equals("") ?
session.getRootNode() : session.getNode(parentPath);
log.info("Creating node {} with primary type {}",
fullPath, psd.getPrimaryType());
- Node node = addChildNode(parent, psd);
- List<String> mixins = psd.getMixins();
- if (mixins != null) {
- log.info("Adding mixins {} to node {}", mixins,
fullPath);
- for (String mixin : mixins) {
- node.addMixin(mixin);
- }
+ node = addChildNode(parent, psd);
+
+ } else {
+ throw new RepoInitException("There is a property with the
name of the to be created node already at " + fullPath + ", therefore bailing
out here as potentially not supported by the underlying JCR");
+ }
+ List<String> mixinsToAdd = new ArrayList<>(psd.getMixins());
+ // don't add mixins already set on the node
Review Comment:
what is the reason for omitting mixins that are already set?
also this changes looks unrelated to the SLING-11736 and i would suggest to
make the change with a separate PR (if really needed)
--
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-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
