Commit a24724de authored by Ate Douma's avatar Ate Douma

CMS7-7024: fixes and improvements from review feedback

- exposing SortedSet/SortedMap collections (which they are) instead of plain Set/Map in the ContentTypeService API
- using more specific getDocumentTypeForNode* methods matching the parameter signature instead of overloading the getDocumentType method name
- fixing an comparison bug in DocumentTypeImpl.merge method
- simplified code and readability by using JcrUtils.get<Type>Property methods
- replace deprecated org.hippoecm.repository.ext.DaemonModule with org.onehippo.repository.modules.DaemonModule interface
parent 98335761
......@@ -19,7 +19,7 @@ package org.onehippo.cms7.services.contenttype;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
import org.hippoecm.repository.ext.DaemonModule;
import org.onehippo.repository.modules.DaemonModule;
import org.onehippo.cms7.services.HippoServiceRegistry;
public class ContentTypeServiceModule implements DaemonModule {
......
......@@ -23,6 +23,7 @@ import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
import org.slf4j.Logger;
......@@ -38,8 +39,8 @@ public class DocumentTypeImpl extends Sealable implements DocumentType {
private EffectiveNodeTypeImpl ent;
private String name;
private String prefix;
private Set<String> superTypes = new TreeSet<String>();
private Set<String> aggregatedTypes = new TreeSet<String>();
private SortedSet<String> superTypes = new TreeSet<String>();
private SortedSet<String> aggregatedTypes = new TreeSet<String>();
private boolean compound;
private boolean mixin;
private boolean template;
......@@ -91,8 +92,8 @@ public class DocumentTypeImpl extends Sealable implements DocumentType {
@Override
protected void doSeal() {
ent.seal();
superTypes = Collections.unmodifiableSet(superTypes);
aggregatedTypes = Collections.unmodifiableSet(aggregatedTypes);
superTypes = Collections.unmodifiableSortedSet(superTypes);
aggregatedTypes = Collections.unmodifiableSortedSet(aggregatedTypes);
for (DocumentTypeField df : fields.values() ) {
((Sealable)df).seal();
}
......@@ -146,11 +147,11 @@ public class DocumentTypeImpl extends Sealable implements DocumentType {
}
@Override
public Set<String> getSuperTypes() {
public SortedSet<String> getSuperTypes() {
return superTypes;
}
public Set<String> getAggregatedTypes() {
public SortedSet<String> getAggregatedTypes() {
return aggregatedTypes;
}
......@@ -248,7 +249,7 @@ public class DocumentTypeImpl extends Sealable implements DocumentType {
// duplicate field name
if (dtf.isMultiple() != entry.getValue().isMultiple() ||
dtf.isPropertyField() != entry.getValue().isPropertyField() ||
dtf.getFieldType() != dtf.getFieldType()) {
dtf.getFieldType().equals(entry.getValue().getFieldType())) {
log.error("Conflicting DocumentType field named {} encountered while merging DocumentType {} with {}. Incoming field ignored."
, new String[]{dtf.getName(), getName(), entry.getValue().getName()});
}
......
......@@ -21,6 +21,7 @@ import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
import java.util.TreeMap;
import javax.jcr.PropertyType;
......@@ -51,7 +52,7 @@ class DocumentTypesCache extends Sealable implements DocumentTypes {
private final EffectiveNodeTypesCache entCache;
private Map<String, String> propertyTypeMappings = new HashMap<String, String>(jcrPropertyTypesMap);
private Map<String, DocumentTypeImpl> types = new TreeMap<String, DocumentTypeImpl>();
private Map<String, Set<DocumentType>> prefixesMap;
private SortedMap<String, Set<DocumentType>> prefixesMap;
protected DocumentTypesCache(EffectiveNodeTypesCache entCache) {
this.entCache = entCache;
......@@ -90,7 +91,7 @@ class DocumentTypesCache extends Sealable implements DocumentTypes {
for (Map.Entry<String, Set<DocumentType>> entry : prefixesMap.entrySet()) {
entry.setValue(Collections.unmodifiableSet(entry.getValue()));
}
prefixesMap = Collections.unmodifiableMap(prefixesMap);
prefixesMap = Collections.unmodifiableSortedMap(prefixesMap);
}
@Override
......@@ -104,7 +105,7 @@ class DocumentTypesCache extends Sealable implements DocumentTypes {
}
@Override
public Map<String, Set<DocumentType>> getTypesByPrefix() {
public SortedMap<String, Set<DocumentType>> getTypesByPrefix() {
return prefixesMap;
}
......
......@@ -18,19 +18,19 @@ package org.onehippo.cms7.services.contenttype;
import java.util.Collections;
import java.util.Iterator;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
public class EffectiveNodeTypeChildImpl extends EffectiveNodeTypeItemImpl implements EffectiveNodeTypeChild {
private String defaultPrimaryType;
private String type;
private Set<String> requiredPrimaryTypes = new TreeSet<String>();
private SortedSet<String> requiredPrimaryTypes = new TreeSet<String>();
@Override
protected void doSeal() {
super.doSeal();
requiredPrimaryTypes = Collections.unmodifiableSet(requiredPrimaryTypes);
requiredPrimaryTypes = Collections.unmodifiableSortedSet(requiredPrimaryTypes);
}
public EffectiveNodeTypeChildImpl(String name, String definingType) {
......@@ -47,7 +47,7 @@ public class EffectiveNodeTypeChildImpl extends EffectiveNodeTypeItemImpl implem
}
@Override
public Set<String> getRequiredPrimaryTypes() {
public SortedSet<String> getRequiredPrimaryTypes() {
return requiredPrimaryTypes;
}
......
......@@ -22,7 +22,8 @@ import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
import java.util.SortedSet;
import java.util.TreeMap;
import java.util.TreeSet;
......@@ -32,8 +33,8 @@ public class EffectiveNodeTypeImpl extends Sealable implements EffectiveNodeType
private String name;
private String prefix;
private Set<String> superTypes = new TreeSet<String>();
private Set<String> aggregatedTypes = new TreeSet<String>();
private SortedSet<String> superTypes = new TreeSet<String>();
private SortedSet<String> aggregatedTypes = new TreeSet<String>();
private boolean aggregate;
private boolean abstractType;
private boolean mixin;
......@@ -41,8 +42,8 @@ public class EffectiveNodeTypeImpl extends Sealable implements EffectiveNodeType
private String primaryItemName;
private Map<String, List<EffectiveNodeTypeChild>> children = new TreeMap<String, List<EffectiveNodeTypeChild>>();
private Map<String, List<EffectiveNodeTypeProperty>> properties = new TreeMap<String, List<EffectiveNodeTypeProperty>>();
private SortedMap<String, List<EffectiveNodeTypeChild>> children = new TreeMap<String, List<EffectiveNodeTypeChild>>();
private SortedMap<String, List<EffectiveNodeTypeProperty>> properties = new TreeMap<String, List<EffectiveNodeTypeProperty>>();
private static final Comparator<EffectiveNodeTypeProperty> propertyComparator = new Comparator<EffectiveNodeTypeProperty>() {
@Override
......@@ -98,8 +99,8 @@ public class EffectiveNodeTypeImpl extends Sealable implements EffectiveNodeType
@Override
protected void doSeal() {
superTypes = Collections.unmodifiableSet(superTypes);
aggregatedTypes = Collections.unmodifiableSet(aggregatedTypes);
superTypes = Collections.unmodifiableSortedSet(superTypes);
aggregatedTypes = Collections.unmodifiableSortedSet(aggregatedTypes);
for (Map.Entry<String, List<EffectiveNodeTypeChild>> entry : children.entrySet()) {
Collections.sort(entry.getValue(),childComparator);
entry.setValue(Collections.unmodifiableList(entry.getValue()));
......@@ -112,8 +113,8 @@ public class EffectiveNodeTypeImpl extends Sealable implements EffectiveNodeType
for (EffectiveNodeTypeProperty i : entry.getValue())
((Sealable)i).seal();
}
children = Collections.unmodifiableMap(children);
properties = Collections.unmodifiableMap(properties);
children = Collections.unmodifiableSortedMap(children);
properties = Collections.unmodifiableSortedMap(properties);
}
@Override
......@@ -127,7 +128,7 @@ public class EffectiveNodeTypeImpl extends Sealable implements EffectiveNodeType
}
@Override
public Set<String> getAggregatedTypes() {
public SortedSet<String> getAggregatedTypes() {
return aggregatedTypes;
}
......@@ -151,7 +152,7 @@ public class EffectiveNodeTypeImpl extends Sealable implements EffectiveNodeType
}
@Override
public Set<String> getSuperTypes() {
public SortedSet<String> getSuperTypes() {
return superTypes;
}
......@@ -201,12 +202,12 @@ public class EffectiveNodeTypeImpl extends Sealable implements EffectiveNodeType
}
@Override
public Map<String, List<EffectiveNodeTypeChild>> getChildren() {
public SortedMap<String, List<EffectiveNodeTypeChild>> getChildren() {
return children;
}
@Override
public Map<String, List<EffectiveNodeTypeProperty>> getProperties() {
public SortedMap<String, List<EffectiveNodeTypeProperty>> getProperties() {
return properties;
}
......
......@@ -20,6 +20,7 @@ import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
import java.util.TreeMap;
class EffectiveNodeTypesCache extends Sealable implements EffectiveNodeTypes {
......@@ -29,7 +30,7 @@ class EffectiveNodeTypesCache extends Sealable implements EffectiveNodeTypes {
private final long version = ++versionSequence;
private Map<String, EffectiveNodeTypeImpl> types = new TreeMap<String, EffectiveNodeTypeImpl>();
private Map<String, Set<EffectiveNodeType>> prefixesMap;
private SortedMap<String, Set<EffectiveNodeType>> prefixesMap;
protected Map<String, EffectiveNodeTypeImpl> getTypes() {
return types;
......@@ -55,7 +56,7 @@ class EffectiveNodeTypesCache extends Sealable implements EffectiveNodeTypes {
entry.setValue(Collections.unmodifiableSet(entry.getValue()));
}
prefixesMap = Collections.unmodifiableMap(prefixesMap);
prefixesMap = Collections.unmodifiableSortedMap(prefixesMap);
}
@Override
......@@ -69,7 +70,7 @@ class EffectiveNodeTypesCache extends Sealable implements EffectiveNodeTypes {
}
@Override
public Map<String, Set<EffectiveNodeType>> getTypesByPrefix() {
public SortedMap<String, Set<EffectiveNodeType>> getTypesByPrefix() {
return prefixesMap;
}
}
......@@ -41,6 +41,7 @@ import javax.jcr.observation.EventIterator;
import javax.jcr.observation.EventListener;
import org.hippoecm.repository.api.HippoNodeType;
import org.hippoecm.repository.util.JcrUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
......@@ -123,7 +124,7 @@ public class HippoContentTypeService implements ContentTypeService {
}
@Override
public DocumentType getDocumentType(Node node) throws RepositoryException {
public DocumentType getDocumentTypeForNode(Node node) throws RepositoryException {
AggregatedDocumentTypesCache adtCache = getAggregatedDocumentTypes();
DocumentType dt = null;
Set<String> names = new HashSet<String>();
......@@ -136,13 +137,13 @@ public class HippoContentTypeService implements ContentTypeService {
}
@Override
public DocumentType getDocumentType(String uuid, Session session) throws RepositoryException {
return getDocumentType(session.getNodeByIdentifier(uuid));
public DocumentType getDocumentTypeForNodeByUuid(Session session, String uuid) throws RepositoryException {
return getDocumentTypeForNode(session.getNodeByIdentifier(uuid));
}
@Override
public DocumentType getDocumentType(Session session, String path) throws RepositoryException {
return getDocumentType(session.getNode(path));
public DocumentType getDocumentTypeForNodeByPath(Session session, String path) throws RepositoryException {
return getDocumentTypeForNode(session.getNode(path));
}
private EffectiveNodeTypesCache loadEffectiveNodeTypes(Session session, boolean allowRetry) throws RepositoryException {
......@@ -288,11 +289,10 @@ public class HippoContentTypeService implements ContentTypeService {
for (NodeIterator typeIterator = handle.getNodes(HippoNodeType.NT_NODETYPE); typeIterator.hasNext(); ) {
typeNode = typeIterator.nextNode();
if (typeNode.isNodeType(HippoNodeType.NT_REMODEL)) {
if (typeNode.hasProperty(HippoNodeType.HIPPO_NODE) // use-case: hippo:namespaces/hippo:document doesn't have NIPPO_NODE property
&& !typeNode.getProperty(HippoNodeType.HIPPO_NODE).getBoolean()) {
// use-case: hippo:namespaces/hippo:document doesn't have NIPPO_NODE property
if (!JcrUtils.getBooleanProperty(typeNode, HippoNodeType.HIPPO_NODE, true)) {
String typeAlias = ("system".equals(namespacePrefix.getName()) ? "" : namespacePrefix.getName()+":") + typeTemplate.getName();
String jcrType = typeNode.hasProperty(HippoNodeType.HIPPOSYSEDIT_TYPE)
? typeNode.getProperty(HippoNodeType.HIPPOSYSEDIT_TYPE).getString() : typeTemplate.getName();
String jcrType = JcrUtils.getStringProperty(typeNode, HippoNodeType.HIPPOSYSEDIT_TYPE, typeTemplate.getName());
try {
PropertyType.valueFromName(jcrType);
dtCache.getPropertyTypeMappings().put(typeAlias, jcrType);
......@@ -386,9 +386,8 @@ public class HippoContentTypeService implements ContentTypeService {
private void loadDocumentType(String typeName, Node typeNode, DocumentTypesCache dtCache) throws RepositoryException {
DocumentTypeImpl dt = dtCache.getTypes().get(typeName);
if (typeNode.hasProperty(HippoNodeType.HIPPO_MIXIN)) {
dt.setMixin(typeNode.getProperty(HippoNodeType.HIPPO_MIXIN).getBoolean());
}
dt.setMixin(JcrUtils.getBooleanProperty(typeNode, HippoNodeType.HIPPO_MIXIN, false));
if (typeNode.hasProperty(HippoNodeType.HIPPO_SUPERTYPE)) {
Value[] values = typeNode.getProperty(HippoNodeType.HIPPO_SUPERTYPE).getValues();
for (Value value : values) {
......@@ -402,9 +401,7 @@ public class HippoContentTypeService implements ContentTypeService {
}
}
}
if (typeNode.hasProperty(HippoNodeType.HIPPO_CASCADEVALIDATION)) {
dt.setCascadeValidate(typeNode.getProperty(HippoNodeType.HIPPO_CASCADEVALIDATION).getBoolean());
}
dt.setCascadeValidate(JcrUtils.getBooleanProperty(typeNode, HippoNodeType.HIPPO_CASCADEVALIDATION, false));
if (typeNode.hasNodes()) {
for (NodeIterator fieldsIterator = typeNode.getNodes(); fieldsIterator.hasNext(); ) {
......@@ -421,13 +418,9 @@ public class HippoContentTypeService implements ContentTypeService {
// DocumentType model doesn't support residual fields
continue;
}
if (!field.hasProperty(HippoNodeType.HIPPOSYSEDIT_TYPE)) {
// TODO: log warn invalid/undefined field type
fieldType = PropertyType.TYPENAME_STRING;
}
else {
fieldType = field.getProperty(HippoNodeType.HIPPOSYSEDIT_TYPE).getString();
}
fieldType = JcrUtils.getStringProperty(field, HippoNodeType.HIPPOSYSEDIT_TYPE, PropertyType.TYPENAME_STRING);
if (dtCache.getPropertyTypeMappings().containsKey(fieldType)) {
df = new DocumentTypeFieldImpl(dt.getName(), fieldName, fieldType, dtCache.getPropertyTypeMappings().get(fieldType));
}
......@@ -441,24 +434,13 @@ public class HippoContentTypeService implements ContentTypeService {
// TODO: log warn unknown fieldType value
continue;
}
if (field.hasProperty(HippoNodeType.HIPPO_MANDATORY)) {
df.setMandatory(field.getProperty(HippoNodeType.HIPPO_MANDATORY).getBoolean());
}
if (field.hasProperty(HippoNodeType.HIPPO_AUTOCREATED)) {
df.setAutoCreated(field.getProperty(HippoNodeType.HIPPO_AUTOCREATED).getBoolean());
}
if (field.hasProperty(HippoNodeType.HIPPO_MULTIPLE)) {
df.setMultiple(field.getProperty(HippoNodeType.HIPPO_MULTIPLE).getBoolean());
}
if (field.hasProperty(HippoNodeType.HIPPO_ORDERED)) {
df.setOrdered(field.getProperty(HippoNodeType.HIPPO_ORDERED).getBoolean());
}
if (field.hasProperty(HippoNodeType.HIPPO_PROTECTED)) {
df.setProtected(field.getProperty(HippoNodeType.HIPPO_PROTECTED).getBoolean());
}
if (field.hasProperty(HippoNodeType.HIPPO_PRIMARY)) {
df.setPrimaryField(field.getProperty(HippoNodeType.HIPPO_PRIMARY).getBoolean());
}
df.setMandatory(JcrUtils.getBooleanProperty(field, HippoNodeType.HIPPO_MANDATORY, false));
df.setAutoCreated(JcrUtils.getBooleanProperty(field, HippoNodeType.HIPPO_AUTOCREATED, false));
df.setMultiple(JcrUtils.getBooleanProperty(field, HippoNodeType.HIPPO_MULTIPLE, false));
df.setOrdered(JcrUtils.getBooleanProperty(field, HippoNodeType.HIPPO_ORDERED, false));
df.setProtected(JcrUtils.getBooleanProperty(field, HippoNodeType.HIPPO_PROTECTED, false));
df.setPrimaryField(JcrUtils.getBooleanProperty(field, HippoNodeType.HIPPO_PRIMARY, false));
if (field.hasProperty(HippoNodeType.HIPPO_VALIDATORS)) {
Value[] values = field.getProperty(HippoNodeType.HIPPO_VALIDATORS).getValues();
for (Value value : values) {
......
......@@ -269,7 +269,7 @@ public class HippoContentTypeServiceTest extends PluginTest {
session.getRootNode().addNode("testNode", "test:test");
session.save();
t = service.getDocumentType(session, "/testNode");
t = service.getDocumentTypeForNodeByPath(session, "/testNode");
assertEquals(4, t.getFields().size());
assertEquals(1, t.getAggregatedTypes().size());
......@@ -277,7 +277,7 @@ public class HippoContentTypeServiceTest extends PluginTest {
session.getNode("/testNode").addMixin("hippostd:relaxed");
session.save();
t = service.getDocumentType(session, "/testNode");
t = service.getDocumentTypeForNodeByPath(session, "/testNode");
assertEquals(5, t.getFields().size());
assertTrue(t.getFields().containsKey("test:extraField"));
assertTrue(t.getAggregatedTypes().contains("hippostd:relaxed"));
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment