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

        Antonio Sanso created issue -
        Hide
        Antonio Sanso added a comment -

        attaching proposed patch.

        Tests to follow

        Show
        Antonio Sanso added a comment - attaching proposed patch. Tests to follow
        Antonio Sanso made changes -
        Field Original Value New Value
        Attachment SLING-2913-patch.txt [ 12586515 ]
        Gavin made changes -
        Workflow no-reopen-closed,doc-test-required [ 12785854 ] re-open possible,doc-test-required [ 12790228 ]
        Gavin made changes -
        Workflow re-open possible,doc-test-required [ 12790228 ] no-reopen-closed,doc-test-required [ 12793144 ]
        Carsten Ziegeler made changes -
        Fix Version/s Servlets Post 2.3.2 [ 12324380 ]
        Carsten Ziegeler made changes -
        Assignee Carsten Ziegeler [ cziegeler ]
        Carsten Ziegeler made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Carsten Ziegeler made changes -
        Status In Progress [ 3 ] Open [ 1 ]
        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)
        Carsten Ziegeler made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Carsten Ziegeler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development