[MRM-1327] remove unnecessary facet node for project versions, and ensure artifact facet properties are correctly removed

git-svn-id: https://svn.apache.org/repos/asf/archiva/trunk@1051444 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 68 insertions, 32 deletions

diff --git a/archiva-modules/metadata/metadata-repository-api/src/test/java/org/apache/archiva/metadata/repository/AbstractMetadataRepositoryTest.java b/archiva-modules/metadata/metadata-repository-api/src/test/java/org/apache/archiva/metadata/repository/AbstractMetadataRepositoryTest.java
index afd34b363..e96c00d01 100644
--- a/archiva-modules/metadata/metadata-repository-api/src/test/java/org/apache/archiva/metadata/repository/AbstractMetadataRepositoryTest.java
+++ b/archiva-modules/metadata/metadata-repository-api/src/test/java/org/apache/archiva/metadata/repository/AbstractMetadataRepositoryTest.java
@@ -432,6 +432,41 @@ public abstract class AbstractMetadataRepositoryTest
         assertFalse( testFacet.toProperties().containsKey( "deleteKey" ) );
     }
 
+    public void testUpdateArtifactMetadataWithExistingFacetsFacetPropertyWasRemoved()
+        throws MetadataResolutionException
+    {
+        ArtifactMetadata metadata = createArtifact();
+
+        Map<String, String> additionalProps = new HashMap<String, String>();
+        additionalProps.put( "deleteKey", "deleteValue" );
+
+        MetadataFacet facet = new TestMetadataFacet( TEST_FACET_ID, "baz", additionalProps );
+        metadata.addFacet( facet );
+        repository.updateArtifact( TEST_REPO_ID, TEST_NAMESPACE, TEST_PROJECT, TEST_PROJECT_VERSION, metadata );
+
+        metadata = repository.getArtifacts( TEST_REPO_ID, TEST_NAMESPACE, TEST_PROJECT,
+                                            TEST_PROJECT_VERSION ).iterator().next();
+        assertEquals( Collections.singleton( TEST_FACET_ID ), metadata.getFacetIds() );
+
+        TestMetadataFacet testFacet = (TestMetadataFacet) metadata.getFacet( TEST_FACET_ID );
+        Map<String, String> facetProperties = testFacet.toProperties();
+
+        assertEquals( "deleteValue", facetProperties.get( "deleteKey" ) );
+
+        facetProperties.remove( "deleteKey" );
+
+        TestMetadataFacet newTestFacet = new TestMetadataFacet( TEST_FACET_ID, testFacet.getValue(), facetProperties );
+        metadata.addFacet( newTestFacet );
+
+        repository.updateArtifact( TEST_REPO_ID, TEST_NAMESPACE, TEST_PROJECT, TEST_PROJECT_VERSION, metadata );
+
+        metadata = repository.getArtifacts( TEST_REPO_ID, TEST_NAMESPACE, TEST_PROJECT,
+                                            TEST_PROJECT_VERSION ).iterator().next();
+        assertEquals( Collections.singleton( TEST_FACET_ID ), metadata.getFacetIds() );
+        testFacet = (TestMetadataFacet) metadata.getFacet( TEST_FACET_ID );
+        assertFalse( testFacet.toProperties().containsKey( "deleteKey" ) );
+    }
+
     public void testUpdateArtifactMetadataWithExistingFacets()
     {
         ArtifactMetadata metadata = createArtifact();
diff --git a/archiva-modules/plugins/metadata-repository-file/src/main/java/org/apache/archiva/metadata/repository/file/FileMetadataRepository.java b/archiva-modules/plugins/metadata-repository-file/src/main/java/org/apache/archiva/metadata/repository/file/FileMetadataRepository.java
index d03f756ba..58c0aa3c4 100644
--- a/archiva-modules/plugins/metadata-repository-file/src/main/java/org/apache/archiva/metadata/repository/file/FileMetadataRepository.java
+++ b/archiva-modules/plugins/metadata-repository-file/src/main/java/org/apache/archiva/metadata/repository/file/FileMetadataRepository.java
@@ -142,7 +142,7 @@ public class FileMetadataRepository
             }
 
             // clear the facet contents so old properties are no longer written
-            clearMetadataFacetProperties( versionMetadata, properties );
+            clearMetadataFacetProperties( versionMetadata.getFacetList(), properties, "" );
         }
         properties.setProperty( "id", versionMetadata.getId() );
         setProperty( properties, "name", versionMetadata.getName() );
@@ -221,29 +221,30 @@ public class FileMetadataRepository
     {
         for ( MetadataFacet facet : versionMetadata.getFacetList() )
         {
-            for ( Map.Entry<String,String> entry : facet.toProperties().entrySet() )
+            for ( Map.Entry<String, String> entry : facet.toProperties().entrySet() )
             {
                 properties.setProperty( facet.getFacetId() + ":" + entry.getKey(), entry.getValue() );
             }
         }
     }
 
-    private void clearMetadataFacetProperties( ProjectVersionMetadata versionMetadata, Properties properties )
+    private static void clearMetadataFacetProperties( Collection<MetadataFacet> facetList, Properties properties,
+                                                      String prefix )
     {
         List<Object> propsToRemove = new ArrayList<Object>();
-        for ( MetadataFacet facet : versionMetadata.getFacetList() )
+        for ( MetadataFacet facet : facetList )
         {
             for ( Object key : properties.keySet() )
             {
-                String keyString = ( String ) key;
-                if( keyString.startsWith( facet.getFacetId() + ":" ) )
+                String keyString = (String) key;
+                if ( keyString.startsWith( prefix + facet.getFacetId() + ":" ) )
                 {
                     propsToRemove.add( key );
                 }
             }
         }
 
-        for( Object key : propsToRemove )
+        for ( Object key : propsToRemove )
         {
             properties.remove( key );
         }
@@ -674,6 +675,8 @@ public class FileMetadataRepository
 
         Properties properties = readOrCreateProperties( directory, PROJECT_VERSION_METADATA_KEY );
 
+        clearMetadataFacetProperties( artifact.getFacetList(), properties, "artifact:facet:" + artifact.getId() + ":" );
+
         String id = artifact.getId();
         properties.setProperty( "artifact:updated:" + id, Long.toString( artifact.getFileLastModified().getTime() ) );
         properties.setProperty( "artifact:whenGathered:" + id, Long.toString( artifact.getWhenGathered().getTime() ) );
diff --git a/archiva-modules/plugins/metadata-store-jcr/src/main/java/org/apache/archiva/metadata/repository/jcr/JcrMetadataRepository.java b/archiva-modules/plugins/metadata-store-jcr/src/main/java/org/apache/archiva/metadata/repository/jcr/JcrMetadataRepository.java
index 7a8506cea..9d8321242 100644
--- a/archiva-modules/plugins/metadata-store-jcr/src/main/java/org/apache/archiva/metadata/repository/jcr/JcrMetadataRepository.java
+++ b/archiva-modules/plugins/metadata-store-jcr/src/main/java/org/apache/archiva/metadata/repository/jcr/JcrMetadataRepository.java
@@ -56,7 +56,6 @@ import javax.jcr.Node;
 import javax.jcr.NodeIterator;
 import javax.jcr.PathNotFoundException;
 import javax.jcr.Property;
-import javax.jcr.PropertyIterator;
 import javax.jcr.Repository;
 import javax.jcr.RepositoryException;
 import javax.jcr.Session;
@@ -78,12 +77,12 @@ import javax.jcr.query.QueryResult;
 public class JcrMetadataRepository
     implements MetadataRepository
 {
-    private static final String ARTIFACT_FACET_NODE_TYPE = "archiva:artifactFacet";
-
     private static final String JCR_LAST_MODIFIED = "jcr:lastModified";
 
     private static final String ARTIFACT_NODE_TYPE = "archiva:artifact";
 
+    private static final String FACET_NODE_TYPE = "archiva:facet";
+
     /**
      * @plexus.requirement role="org.apache.archiva.metadata.model.MetadataFacetFactory"
      */
@@ -114,7 +113,7 @@ public class JcrMetadataRepository
 
             NodeTypeManager nodeTypeManager = workspace.getNodeTypeManager();
             registerMixinNodeType( nodeTypeManager, ARTIFACT_NODE_TYPE );
-            registerMixinNodeType( nodeTypeManager, ARTIFACT_FACET_NODE_TYPE );
+            registerMixinNodeType( nodeTypeManager, FACET_NODE_TYPE );
         }
         catch ( LoginException e )
         {
@@ -183,9 +182,14 @@ public class JcrMetadataRepository
 
             for ( MetadataFacet facet : artifactMeta.getFacetList() )
             {
-                // TODO: need to clear it?
-                Node n = JcrUtils.getOrAddNode( node, facet.getFacetId() );
-                n.addMixin( ARTIFACT_FACET_NODE_TYPE );
+                if ( node.hasNode( facet.getFacetId() ) )
+                {
+                    node.getNode( facet.getFacetId() ).remove();
+                }
+
+                // recreate, to ensure properties are removed
+                Node n = node.addNode( facet.getFacetId() );
+                n.addMixin( FACET_NODE_TYPE );
 
                 for ( Map.Entry<String, String> entry : facet.toProperties().entrySet() )
                 {
@@ -271,16 +275,15 @@ public class JcrMetadataRepository
                 i++;
             }
 
-            // TODO: namespaced properties instead?
-            Node facetNode = JcrUtils.getOrAddNode( versionNode, "facets" );
             for ( MetadataFacet facet : versionMetadata.getFacetList() )
             {
-                // TODO: shouldn't need to recreate, just update
-                if ( facetNode.hasNode( facet.getFacetId() ) )
+                // recreate, to ensure properties are removed
+                if ( versionNode.hasNode( facet.getFacetId() ) )
                 {
-                    facetNode.getNode( facet.getFacetId() ).remove();
+                    versionNode.getNode( facet.getFacetId() ).remove();
                 }
-                Node n = facetNode.addNode( facet.getFacetId() );
+                Node n = versionNode.addNode( facet.getFacetId() );
+                n.addMixin( FACET_NODE_TYPE );
 
                 for ( Map.Entry<String, String> entry : facet.toProperties().entrySet() )
                 {
@@ -853,27 +856,22 @@ public class JcrMetadataRepository
                 i++;
             }
 
-            if ( node.hasNode( "facets" ) )
+            for ( Node n : JcrUtils.getChildNodes( node ) )
             {
-                NodeIterator j = node.getNode( "facets" ).getNodes();
-
-                while ( j.hasNext() )
+                if ( n.isNodeType( FACET_NODE_TYPE ) )
                 {
-                    Node facetNode = j.nextNode();
-
-                    MetadataFacetFactory factory = metadataFacetFactories.get( facetNode.getName() );
+                    String name = n.getName();
+                    MetadataFacetFactory factory = metadataFacetFactories.get( name );
                     if ( factory == null )
                     {
-                        log.error( "Attempted to load unknown project version metadata facet: " + facetNode.getName() );
+                        log.error( "Attempted to load unknown project version metadata facet: " + name );
                     }
                     else
                     {
                         MetadataFacet facet = factory.createMetadataFacet();
                         Map<String, String> map = new HashMap<String, String>();
-                        PropertyIterator iterator = facetNode.getProperties();
-                        while ( iterator.hasNext() )
+                        for ( Property property : JcrUtils.getProperties( n ) )
                         {
-                            Property property = iterator.nextProperty();
                             String p = property.getName();
                             if ( !p.startsWith( "jcr:" ) )
                             {
@@ -1144,7 +1142,7 @@ public class JcrMetadataRepository
 
         for ( Node n : JcrUtils.getChildNodes( artifactNode ) )
         {
-            if ( n.isNodeType( ARTIFACT_FACET_NODE_TYPE ) )
+            if ( n.isNodeType( FACET_NODE_TYPE ) )
             {
                 String name = n.getName();
                 MetadataFacetFactory factory = metadataFacetFactories.get( name );