Skip to content

Added ODM Metadata Inheritance fixes #340 #591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Added ODM Metadata Inheritance fixes #340 #591

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b009d28
Added ODM Metadata Inheritance fixes #340
Rob4001 Nov 25, 2021
bb67501
Merge branch 'spring-projects:main' into odm-inheritance
Rob4001 Jun 3, 2022
d29c1b2
Update Copyright dates
Rob4001 Jun 3, 2022
2b3048b
Accidentally modified wrong Copyright file
Rob4001 Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 44 additions & 35 deletions core/src/main/java/org/springframework/ldap/odm/core/impl/ObjectMetaData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.core.annotation.AnnotatedElementUtils;
import org.springframework.ldap.odm.annotations.Entry;
import org.springframework.ldap.odm.annotations.Id;
import org.springframework.ldap.support.LdapUtils;
import org.springframework.util.ReflectionUtils;
import org.springframework.util.StringUtils;

import javax.naming.Name;
Expand Down Expand Up @@ -85,29 +87,33 @@ public AttributeMetaData getAttribute(Field field) {
return fieldToAttribute.get(field);
}

public ObjectMetaData(Class<?> clazz) {
public ObjectMetaData(final Class<?> clazz) {
if (LOG.isDebugEnabled()) {
LOG.debug(String.format("Extracting metadata from %1$s", clazz));
}

// Get object class metadata - the @Entity annotation
Entry entity = clazz.getAnnotation(Entry.class);
if (entity != null) {
// findAllMergedAnnotations will return set of inherited annotations and apply them from superclass down to subclass
Set<Entry> entities = AnnotatedElementUtils.findAllMergedAnnotations(clazz,Entry.class);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree that we want to resolve inheritance and meta-annotation concerns, I don't think we want to merge multiple Entry annotations. The reason is that each Entry can come with a base and it's not clear which should be picked.

Instead, it may be valuable to look at Spring Security's AuthorizationAnnotationUtil#findUniqueAnnotation for some inspiration on how to resolve the @Entry annotation while still erroring if the result is ambiguous.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a use case where say you have a inetOrgPerson class which inherits from a organizationalPerson class which inherits from a person class. If you want to reference inetOrgPersons in one query, but person in another they could have different Entry annotation values. By enumerating through all annotations from the top of the class tree and overriding with subclasses it still allows this behavior.

Copy link
Collaborator

@jzheaux jzheaux Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the use case for filters, but the base value remains ambiguous. Which base should you pick? More importantly, how do you reconcile the fact that you pick a base from one @Entry but all the filters from all @Entry annotations?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long absence. I would argue that the default should be to take the lowest class in the class inheritance tree (closest to the instantiated class type) with the ability for the class writer to disambiguate by defining the base on the subclass.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowest class

I agree with this, and I think it should apply consistently for all of Entry's properties. Instead of picking the lowest base and merging all objectClasses in the type hierarchy, the code should pick the lowest Entry and use its base and objectClasses.

with the ability for the class writer to disambiguate by defining the base on the subclass.

This should also apply for objectClasses. Otherwise, it is tricky for the subclass to override objectClasses in the superclasses.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to get hung up on this feature, and it merits further discussion. Could we create a separate ticket to discuss how objectClasses inheritance should work? I'm happy to add to that ticket my thoughts and we can continue the conversation over there.

For this ticket, can you please leave inheritance as follows:

  1. The target class must have an @Entry annotation or meta-annotation
  2. No parent @Entry annotations are considered
  3. All reachable fields are collected (what you are already doing here)

The reason for keeping 3 is that it's clear from the ticket that not collecting all reachable fields is the thing that is primarily surprising to folks.

I'm also happy to update the PR with a polish that does the above if you aren't available. If I don't hear from you within a month or so, I'll proceed as I've described so that it can land in time for the 3.3.0 release.

if (entities != null && !entities.isEmpty()) {
// Default objectclass name to the class name unless it's specified
// in @Entity(name={objectclass1, objectclass2});
String[] localObjectClasses = entity.objectClasses();
if (localObjectClasses != null && localObjectClasses.length > 0 && localObjectClasses[0].length() > 0) {
for (String localObjectClass:localObjectClasses) {
objectClasses.add(new CaseIgnoreString(localObjectClass));
for (Entry entity: entities) {
String[] localObjectClasses = entity.objectClasses();
if (localObjectClasses.length > 0 && localObjectClasses[0].length() > 0) {
for (String localObjectClass : localObjectClasses) {
objectClasses.add(new CaseIgnoreString(localObjectClass));
}
}
} else {
String base = entity.base();
if (StringUtils.hasText(base)) {
this.base = LdapUtils.newLdapName(base);
}
}
if(objectClasses.isEmpty()) {
objectClasses.add(new CaseIgnoreString(clazz.getSimpleName()));
}

String base = entity.base();
if(StringUtils.hasText(base)) {
this.base = LdapUtils.newLdapName(base);
}
} else {
throw new MetaDataException(String.format("Class %1$s must have a class level %2$s annotation", clazz,
Entry.class));
Expand All @@ -119,31 +125,34 @@ public ObjectMetaData(Class<?> clazz) {
}

// Get field meta-data - the @Attribute annotation
Field[] fields = clazz.getDeclaredFields();
for (Field field : fields) {
// So we can write to private fields
field.setAccessible(true);

// Skip synthetic or static fields
if (Modifier.isStatic(field.getModifiers()) || field.isSynthetic()) {
continue;
}

AttributeMetaData currentAttributeMetaData=new AttributeMetaData(field);
if (currentAttributeMetaData.isId()) {
if (idAttribute!=null) {
// There can be only one id field
throw new MetaDataException(
String.format("You man have only one field with the %1$s annotation in class %2$s", Id.class, clazz));
ReflectionUtils.doWithFields(clazz, new ReflectionUtils.FieldCallback() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me the purpose of this change to use doWithFields. Is this change necessary to address ODM inheritance?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, clazz.getDeclaredFields(); will only return fields declaired directly in the class and not any inherited fields. the "doWithFields" method will recurse through the type inheritance tree to get all fields annotateed with spring ldap annotations.

https://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#getDeclaredFields--

@Override
public void doWith(Field field) throws IllegalArgumentException {
// So we can write to private fields
field.setAccessible(true);

// Skip synthetic or static fields
if (Modifier.isStatic(field.getModifiers()) || field.isSynthetic()) {
return;
}

AttributeMetaData currentAttributeMetaData=new AttributeMetaData(field);
if (currentAttributeMetaData.isId()) {
if (idAttribute!=null) {
// There can be only one id field
throw new MetaDataException(
String.format("You man have only one field with the %1$s annotation in class %2$s", Id.class, clazz));
}
idAttribute=currentAttributeMetaData;
}
fieldToAttribute.put(field, currentAttributeMetaData);

if(currentAttributeMetaData.isDnAttribute()) {
dnAttributes.add(currentAttributeMetaData);
}
}
idAttribute=currentAttributeMetaData;
}
fieldToAttribute.put(field, currentAttributeMetaData);

if(currentAttributeMetaData.isDnAttribute()) {
dnAttributes.add(currentAttributeMetaData);
}
}
);

if (idAttribute == null) {
throw new MetaDataException(
Expand Down
36 changes: 36 additions & 0 deletions ...rc/test/java/org/springframework/ldap/odm/core/impl/DefaultObjectDirectoryMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,42 @@ public void testMapping() {
assertField(entityData, "entryUUID", "entryUUID", null, false, false, false, true);
}

@Test
public void testMappingInherited() {
assertThat(tested.manageClass(UnitTestWorker.class))
.containsOnlyElementsOf(Arrays.asList("dn", "cn", "sn", "description", "telephoneNumber", "entryUUID", "objectclass", "workerId"));

DefaultObjectDirectoryMapper.EntityData entityData = tested.getMetaDataMap().get(UnitTestWorker.class);

assertThat(entityData).isNotNull();
assertThat(entityData.ocFilter).isEqualTo(query().
where("objectclass").is("inetOrgPerson")
.and("objectclass").is("organizationalPerson")
.and("objectclass").is("person")
.and("objectclass").is("top")
.and("objectclass").is("worker")
.filter());

assertThat(entityData.metaData).hasSize(9);

AttributeMetaData idAttribute = entityData.metaData.getIdAttribute();
assertThat(idAttribute.getField().getName()).isEqualTo("dn");
assertThat(idAttribute.isId()).isTrue();
assertThat(idAttribute.isBinary()).isFalse();
assertThat(idAttribute.isDnAttribute()).isFalse();
assertThat(idAttribute.isTransient()).isFalse();
assertThat(idAttribute.isCollection()).isFalse();

assertField(entityData, "fullName", "cn", "cn", false, false, false, false);
assertField(entityData, "lastName", "sn", null, false, false, false, false);
assertField(entityData, "description", "description", null, false, false, true, false);
assertField(entityData, "country", null, "c", false, true, false, false);
assertField(entityData, "company", null, "ou", false, true, false, false);
assertField(entityData, "telephoneNumber", "telephoneNumber", null, false, false, false, false);
assertField(entityData, "entryUUID", "entryUUID", null, false, false, false, true);
assertField(entityData, "workerId", "workerId", null, false, false, false, false);
}

@Test
public void testInvalidType() {
try {
Expand Down
30 changes: 30 additions & 0 deletions core/src/test/java/org/springframework/ldap/odm/core/impl/UnitTestWorker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 2005-2013 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.ldap.odm.core.impl;

import org.springframework.ldap.odm.annotations.Attribute;
import org.springframework.ldap.odm.annotations.Entry;

/**
* @author Robert Wilson
*/
@Entry(objectClasses = {"worker"})
public class UnitTestWorker extends UnitTestPerson{
@Attribute(name = "workerId")
private String workerId;
}