Sling
  1. Sling
  2. SLING-2913

Issue in AbstractCreateOperation#deepGetOrCreateNode

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Servlets Post 2.3.2
    • Component/s: Servlets
    • Labels:
      None

      Description

      While updating the AbstractCreateOperation to use new CRUD support in POST servlet has been some lost in translation.

      The previous code looked like

      
          protected Node deepGetOrCreateNode(Session session, String path,
                  Map<String, RequestProperty> reqProperties, List<Modification> changes,
                  VersioningConfiguration versioningConfiguration)
                  throws RepositoryException {
              if (log.isDebugEnabled()) {
                  log.debug("Deep-creating Node '{}'", path);
              }
              if (path == null || !path.startsWith("/")) {
                  throw new IllegalArgumentException("path must be an absolute path.");
              }
              // get the starting node
              String startingNodePath = path;
              Node startingNode = null;
              while (startingNode == null) {
                  if (startingNodePath.equals("/")) {
                      startingNode = session.getRootNode();
                  } else if (session.itemExists(startingNodePath)) {
                      startingNode = (Node) session.getItem(startingNodePath);
                      updateNodeType(session, startingNodePath, reqProperties, changes, versioningConfiguration);
                      updateMixins(session, startingNodePath, reqProperties, changes, versioningConfiguration);
                  } else {
                      int pos = startingNodePath.lastIndexOf('/');
                      if (pos > 0) {
                          startingNodePath = startingNodePath.substring(0, pos);
                      } else {
                          startingNodePath = "/";
                      }
                  }
              }
      

      while the updated is

      protected Resource deepGetOrCreateNode(final ResourceResolver resolver,
                          final String path,
                          final Map<String, RequestProperty> reqProperties,
                          final List<Modification> changes,
                          final VersioningConfiguration versioningConfiguration)
          throws PersistenceException, RepositoryException {
              if (log.isDebugEnabled()) {
                  log.debug("Deep-creating resource '{}'", path);
              }
              if (path == null || !path.startsWith("/")) {
                  throw new IllegalArgumentException("path must be an absolute path.");
              }
              // get the starting resource
              String startingResourcePath = path;
              Resource startingResource = null;
              while (startingResource == null) {
                  if (startingResourcePath.equals("/")) {
                      startingResource = resolver.getResource("/");
                  } else if (resolver.getResource(startingResourcePath) != null) {
                      startingResource = resolver.getResource(startingResourcePath);
                      updateNodeType(resolver, startingResourcePath, reqProperties, changes, versioningConfiguration);
                      updateMixins(resolver, startingResourcePath, reqProperties, changes, versioningConfiguration);
                  } else {
                      int pos = startingResourcePath.lastIndexOf('/');
                      if (pos > 0) {
                          startingResourcePath = startingResourcePath.substring(0, pos);
                      } else {
                          startingResourcePath = "/";
                      }
                  }
              }
      

      The main difference is in that the

      startingNode = session.getRootNode(); could throw a RepositoryException e.g. if the session did not have enough permission while startingResource = resolver.getResource("/"); would just return null.

      This might cause a theoretical infinite loop.

        Activity

        Hide
        Antonio Sanso added a comment -

        attaching proposed patch.

        Tests to follow

        Show
        Antonio Sanso added a comment - attaching proposed patch. Tests to follow
        Hide
        Carsten Ziegeler added a comment -

        Thanks for the patch, Antonio. I've applied it (but tweaked the exception message a little bit)

        Show
        Carsten Ziegeler added a comment - Thanks for the patch, Antonio. I've applied it (but tweaked the exception message a little bit)

          People

          • Assignee:
            Carsten Ziegeler
            Reporter:
            Antonio Sanso
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development