andi-huber commented on code in PR #1019:
URL: https://github.com/apache/isis/pull/1019#discussion_r920968718
##########
extensions/core/commandlog/applib/src/main/java/org/apache/isis/extensions/commandlog/applib/contributions/HasInteractionId_commandLogEntry.java:
##########
@@ -0,0 +1,41 @@
+package org.apache.isis.extensions.commandlog.applib.contributions;
Review Comment:
missing license header
##########
extensions/core/commandlog/applib/src/test/java/org/apache/isis/extensions/commandlog/applib/integtest/model/CommandLogTestDomainModel.java:
##########
@@ -0,0 +1,4 @@
+package org.apache.isis.extensions.commandlog.applib.integtest.model;
Review Comment:
missing license header
##########
extensions/core/commandlog/applib/src/test/java/org/apache/isis/extensions/commandlog/applib/integtest/model/Counter.java:
##########
@@ -0,0 +1,50 @@
+package org.apache.isis.extensions.commandlog.applib.integtest.model;
Review Comment:
missing license header
##########
extensions/core/commandlog/persistence-jdo/src/test/java/org/apache/isis/extensions/commandlog/jdo/model/CounterRepository.java:
##########
@@ -0,0 +1,12 @@
+package org.apache.isis.extensions.commandlog.jdo.model;
Review Comment:
missing license header
##########
extensions/core/commandlog/persistence-jpa/src/test/resources/META-INF/persistence.xml:
##########
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review Comment:
missing license header
##########
extensions/core/commandlog/persistence-jpa/src/test/java/org/apache/isis/extensions/commandlog/jpa/model/CounterRepository.java:
##########
@@ -0,0 +1,13 @@
+package org.apache.isis.extensions.commandlog.jpa.model;
Review Comment:
missing license header
##########
extensions/core/executionlog/applib/src/main/java/org/apache/isis/extensions/executionlog/applib/dom/ExecutionLogEntryPK.java:
##########
@@ -0,0 +1,37 @@
+package org.apache.isis.extensions.executionlog.applib.dom;
Review Comment:
missing license header
##########
extensions/core/executionlog/applib/src/main/java/org/apache/isis/extensions/executionlog/applib/dom/ExecutionLogEntryRepository.java:
##########
@@ -0,0 +1,157 @@
+package org.apache.isis.extensions.executionlog.applib.dom;
Review Comment:
missing license header
##########
extensions/core/executionlog/applib/src/main/java/org/apache/isis/extensions/executionlog/applib/dom/ExecutionLogEntryType.java:
##########
@@ -0,0 +1,6 @@
+package org.apache.isis.extensions.executionlog.applib.dom;
Review Comment:
missing license header
##########
api/applib/src/main/java/org/apache/isis/applib/services/bookmark/IdStringifier.java:
##########
@@ -0,0 +1,48 @@
+package org.apache.isis.applib.services.bookmark;
Review Comment:
missing license header
##########
api/applib/src/main/java/org/apache/isis/applib/services/bookmark/IdStringifier.java:
##########
@@ -0,0 +1,48 @@
+package org.apache.isis.applib.services.bookmark;
+
+import java.util.Optional;
+
+import lombok.Getter;
+
+/**
+ * SPI to allow other modules to extend the bookmarking mechanism.
+ *
+ * <p>
+ * Originally introduced to allow JPA CommandLog implementation use
CommandLogEntryPK for its primary key.
+ * </p>
+ */
+public interface IdStringifier<T> {
+
+ boolean handles(Class<?> candidateClass);
+
+ Class<T> getHandledClass();
Review Comment:
in theory could be more than one (handled class)
##########
persistence/jpa/integration/src/main/java/org/apache/isis/persistence/jpa/integration/typeconverters/applib/IsisBookmarkConverter.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://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.apache.isis.persistence.jpa.integration.typeconverters.applib;
+
+import javax.persistence.AttributeConverter;
+import javax.persistence.Converter;
+
+import org.apache.isis.applib.services.bookmark.Bookmark;
+
+/**
+ * @since 2.0 {@index}
+ */
+@Converter(autoApply = true)
+public class IsisBookmarkConverter implements AttributeConverter<Bookmark,
String> {
+
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ public String convertToDatabaseColumn(Bookmark bookmark) {
+ return bookmark != null
+ ? bookmark.toString()
+ : null;
+ }
+
+ @Override
+ public Bookmark convertToEntityAttribute(String datastoreValue) {
Review Comment:
we probably should fail if the bookmark cannot be parsed from non-empty
_datastoreValue_:
```java
public Bookmark convertToEntityAttribute(String datastoreValue) {
return _String.isNotEmpty(datastoreValue)
? Bookmark.parseElseFail(datastoreValue)
: null;
}
```
##########
extensions/core/commandlog/applib/src/test/java/org/apache/isis/extensions/commandlog/applib/integtest/model/CounterRepository.java:
##########
@@ -0,0 +1,31 @@
+package org.apache.isis.extensions.commandlog.applib.integtest.model;
Review Comment:
missing license header
##########
core/metamodel/src/main/java/org/apache/isis/core/metamodel/facets/actions/action/invocation/IdentifierUtil.java:
##########
@@ -109,19 +120,39 @@ public Identifier memberIdentifierFor(
/**
* Whether given command corresponds to given objectMember.
* <p>
- * Is related to {@link #logicalMemberIdentifierFor(ObjectMember)}.
+ * Is related to {@link
#logicalMemberIdentifierForDeclaredMember(ObjectMember)}.
*/
public boolean isCommandForMember(
final @Nullable Command command,
+ final @NonNull InteractionHead interactionHead,
final @Nullable ObjectMember objectMember) {
return command!=null
&& objectMember!=null
- && logicalMemberIdentifierFor(objectMember)
+ && logicalMemberIdentifierFor(interactionHead, objectMember)
.equals(command.getLogicalMemberIdentifier());
}
// -- HELPER
+ private String logicalMemberIdentifierFor(
+ final @NonNull InteractionHead interactionHead,
+ final ObjectMember objectMember) {
+ if (objectMember instanceof ObjectAction) {
+ ObjectAction objectAction = (ObjectAction) objectMember;
+ if (objectAction.isDeclaredOnMixin()) {
+ if (interactionHead instanceof ActionInteractionHead) {
+ ObjectAction objectActionOnMixee =
((ActionInteractionHead) interactionHead).getMetaModel();
+ ObjectSpecification specificationOfMixee =
interactionHead.getOwner().getSpecification();
+ return logicalMemberIdentifierFor(specificationOfMixee,
objectActionOnMixee);
+ }
+ }
+ // we fall through to the declared case, which isn't correct; but
don't think it matters because
Review Comment:
looks correct to me!
maybe I'll revisit this `IdentifierUtil` post merge (and remove ambiguities
if there are any)
##########
extensions/core/commandlog/applib/src/test/java/org/apache/isis/extensions/commandlog/applib/integtest/model/Counter_bumpUsingMixinWithCommandPublishingDisabled.java:
##########
@@ -0,0 +1,17 @@
+package org.apache.isis.extensions.commandlog.applib.integtest.model;
Review Comment:
missing license header
##########
extensions/core/commandlog/applib/src/test/java/org/apache/isis/extensions/commandlog/applib/integtest/model/Counter_bumpUsingMixin.java:
##########
@@ -0,0 +1,17 @@
+package org.apache.isis.extensions.commandlog.applib.integtest.model;
Review Comment:
missing license header
##########
extensions/core/commandlog/persistence-jdo/src/test/java/org/apache/isis/extensions/commandlog/jdo/model/Counter.java:
##########
@@ -0,0 +1,48 @@
+package org.apache.isis.extensions.commandlog.jdo.model;
Review Comment:
missing license header
##########
extensions/core/commandlog/persistence-jpa/src/test/java/org/apache/isis/extensions/commandlog/jpa/model/Counter.java:
##########
@@ -0,0 +1,55 @@
+package org.apache.isis.extensions.commandlog.jpa.model;
Review Comment:
missing license header
##########
extensions/core/executionlog/applib/src/main/java/org/apache/isis/extensions/executionlog/applib/contributions/ExecutionLogEntry_siblingExecutions.java:
##########
@@ -0,0 +1,36 @@
+package org.apache.isis.extensions.executionlog.applib.contributions;
Review Comment:
missing license header
##########
extensions/core/executionlog/applib/src/main/java/org/apache/isis/extensions/executionlog/applib/contributions/HasInteractionId_executionLogEntries.java:
##########
@@ -0,0 +1,33 @@
+package org.apache.isis.extensions.executionlog.applib.contributions;
Review Comment:
missing license header
##########
extensions/core/executionlog/applib/src/main/java/org/apache/isis/extensions/executionlog/applib/dom/ExecutionLogEntry.layout.fallback.xml:
##########
@@ -0,0 +1,97 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
Review Comment:
missing license header
##########
extensions/core/executionlog/applib/src/main/java/org/apache/isis/extensions/executionlog/applib/spiimpl/ExecutionSubscriberForLog.java:
##########
@@ -0,0 +1,24 @@
+package org.apache.isis.extensions.executionlog.applib.spiimpl;
Review Comment:
missing license header
##########
persistence/jpa/integration/src/main/java/org/apache/isis/persistence/jpa/integration/entity/JpaEntityFacetFactory.java:
##########
@@ -393,6 +409,12 @@ private static JpaObjectIdSerializer
createJpaObjectIdSerializer(
return new ByteIdSerializer();
}
}
+ Can<BookmarkService.Stringifier> select =
serviceRegistry.select(BookmarkService.Stringifier.class);
+ for (BookmarkService.Stringifier stringifier : select.toList()) {
Review Comment:
no need to convert `Can` to `List` ... `Can` implements `Iterable`
should just work fine ...
```java
for (BookmarkService.Stringifier stringifier : select) { ...
```
##########
persistence/jpa/integration/src/main/java/org/apache/isis/persistence/jpa/integration/entity/JpaEntityFacetFactory.java:
##########
@@ -393,6 +409,12 @@ private static JpaObjectIdSerializer
createJpaObjectIdSerializer(
return new ByteIdSerializer();
}
}
+ Can<BookmarkService.Stringifier> select =
serviceRegistry.select(BookmarkService.Stringifier.class);
+ for (BookmarkService.Stringifier stringifier : select.toList()) {
+ if(stringifier.handles() == primaryKeyType) {
Review Comment:
If this is the only kind of use for the `stringifier.handles()` SPI method,
I'd suggest to make the SPI more flexible by instead using a predicate like
```java
if(stringifier.handles(primaryKeyType)) ...
```
which would allow a `Stringifier` to handle multiple types (if need be).
If not, this is just fine.
##########
extensions/core/executionlog/applib/src/main/java/org/apache/isis/extensions/executionlog/applib/util/BigDecimalUtils.java:
##########
@@ -0,0 +1,27 @@
+package org.apache.isis.extensions.executionlog.applib.util;
Review Comment:
missing license header
With utility classes, what I usually ask myself: can its scope be made
package private?
if **yes**, then do so and also prefix the class-name with an underscore.
if **no**, maybe this is a candidate for the commons artifact, otherwise
leave it, its fine.
##########
api/applib/src/main/java/org/apache/isis/applib/services/bookmark/IdStringifier.java:
##########
@@ -0,0 +1,48 @@
+package org.apache.isis.applib.services.bookmark;
+
+import java.util.Optional;
+
+import lombok.Getter;
+
+/**
+ * SPI to allow other modules to extend the bookmarking mechanism.
+ *
+ * <p>
+ * Originally introduced to allow JPA CommandLog implementation use
CommandLogEntryPK for its primary key.
+ * </p>
+ */
+public interface IdStringifier<T> {
+
+ boolean handles(Class<?> candidateClass);
+
+ Class<T> getHandledClass();
+
+ String stringify(final T value);
+
+ T parse(final String stringified);
+
+ abstract class Abstract<T> implements IdStringifier<T> {
+
+ @Getter
+ private final Class<T> handledClass;
+ /**
+ * Allows for a Stringifier to handle (for example) both
<code>Integer.class</code> and <code>int.class</code>.
+ */
+ @Getter
+ private final Optional<Class<T>> primitiveClass;
Review Comment:
I guess we could simply imply, eg. that any `IdStringifier` that handles
`Long` must also handle `long`. (Which it trivially does anyway.)
Eg. As is the case for our value-semantics design. We simply imply that
`ValueSemanticsProvider<Long>` also handles the primitive type (`long`).
(Anyway just an idea, might not be applicable here.)
##########
extensions/core/commandlog/persistence-jpa/src/main/java/org/apache/isis/extensions/commandlog/jpa/dom/CommandLogEntryPK.java:
##########
@@ -0,0 +1,57 @@
+package org.apache.isis.extensions.commandlog.jpa.dom;
Review Comment:
missing license header
##########
core/runtimeservices/src/main/java/org/apache/isis/core/runtimeservices/bookmarks/BookmarkServiceDefault.java:
##########
@@ -141,72 +144,47 @@ public Bookmark bookmarkForElseFail(final @Nullable
Object domainObject) {
@Override
public <T> T read(final Class<T> cls, final Serializable value) {
+ if (stringifiers != null) {
+ for (IdStringifier<?> serializer : stringifiers) {
+ if (serializer.handles(cls)) {
+ return (T) serializer.parse((String)value);
+ }
+ }
+ }
+
if(Bookmark.class.equals(cls)) {
return _Casts.uncheckedCast(value);
}
if(Bookmark.class.isAssignableFrom(value.getClass())) {
- final Bookmark valueBookmark = (Bookmark) value;
- return _Casts.uncheckedCast(lookup(valueBookmark).orElse(null));
+ final Bookmark valueAsBookmark = (Bookmark) value;
+ return _Casts.uncheckedCast(lookup(valueAsBookmark).orElse(null));
}
return _Casts.uncheckedCast(value);
}
+ @SuppressWarnings("unchecked")
@Override
public Serializable write(final Object value) {
- if(isPredefinedSerializable(value.getClass())) {
- return (Serializable) value;
- } else {
- return bookmarkForElseFail(value);
+ if (stringifiers != null) {
+ for (IdStringifier stringifier : stringifiers) {
+ if (stringifier.handles(value.getClass())) {
+ return stringified(stringifier, value);
+ }
+ }
}
- }
- // -- HELPER
+ return bookmarkForElseFail(value);
+ }
- private static final Set<Class<? extends Serializable>>
serializableFinalTypes = _Sets.of(
- String.class, String[].class,
- Class.class, Class[].class,
- Character.class, Character[].class, char[].class,
- Boolean.class, Boolean[].class, boolean[].class,
- // Numbers
- Byte[].class, byte[].class,
- Short[].class, short[].class,
- Integer[].class, int[].class,
- Long[].class, long[].class,
- Float[].class, float[].class,
- Double[].class, double[].class
- );
-
- private static final List<Class<? extends Serializable>> serializableTypes
= _Lists.of(
- java.util.Date.class,
- java.sql.Date.class,
- Enum.class,
- Bookmark.class,
- TreeState.class
- );
-
- private static boolean isPredefinedSerializable(final Class<?> cls) {
- if(!Serializable.class.isAssignableFrom(cls)) {
- return false;
- }
- // primitive ... boolean, byte, char, short, int, long, float, and
double.
- if(cls.isPrimitive() || Number.class.isAssignableFrom(cls)) {
- return true;
- }
- //[ahuber] any non-scalar values could be problematic, so we are
careful with wild-cards here
- if(cls.getName().startsWith("java.time.")) {
- return true;
- }
- if(cls.getName().startsWith("org.joda.time.")) {
- return true;
- }
- if(serializableFinalTypes.contains(cls)) {
- return true;
- }
- return serializableTypes.stream().anyMatch(t->t.isAssignableFrom(cls));
+ private static <T> String stringified(IdStringifier<T> stringifier, T
value) {
+ return stringifier.stringify(value);
}
+ // -- HELPER
+
+ @Inject List<IdStringifier<?>> stringifiers;
Review Comment:
The null-safe idiom for that (eg. when JUnit testing) is rather
```java
@Autowired(required = false) List<IdStringifier<?>> stringifiers;
```
Otherwise, in the absence of a/any `IdStringifier` _Spring's_
`ApplicationContext` will fail to initialize.
##########
persistence/jpa/integration/src/main/java/org/apache/isis/persistence/jpa/integration/entity/JpaEntityFacet.java:
##########
@@ -0,0 +1,368 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://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.apache.isis.persistence.jpa.integration.entity;
+
+import java.lang.reflect.Method;
+import java.util.List;
+import java.util.Optional;
+
+import javax.persistence.EntityManager;
+import javax.persistence.PersistenceException;
+import javax.persistence.PersistenceUnitUtil;
+import javax.persistence.metamodel.EntityType;
+
+import org.eclipse.persistence.exceptions.DescriptorException;
+import org.springframework.data.jpa.repository.JpaContext;
+
+import org.apache.isis.applib.exceptions.unrecoverable.ObjectNotFoundException;
+import org.apache.isis.applib.query.AllInstancesQuery;
+import org.apache.isis.applib.query.NamedQuery;
+import org.apache.isis.applib.query.Query;
+import org.apache.isis.applib.services.bookmark.Bookmark;
+import org.apache.isis.applib.services.bookmark.IdStringifier;
+import org.apache.isis.applib.services.registry.ServiceRegistry;
+import org.apache.isis.applib.services.repository.EntityState;
+import org.apache.isis.commons.collections.Can;
+import org.apache.isis.commons.handler.ChainOfResponsibility;
+import org.apache.isis.commons.internal.assertions._Assert;
+import org.apache.isis.commons.internal.base._Casts;
+import org.apache.isis.commons.internal.base._Lazy;
+import org.apache.isis.commons.internal.exceptions._Exceptions;
+import org.apache.isis.core.config.beans.PersistenceStack;
+import org.apache.isis.core.metamodel.facetapi.FacetAbstract;
+import org.apache.isis.core.metamodel.facetapi.FacetHolder;
+import org.apache.isis.core.metamodel.facets.object.entity.EntityFacet;
+import org.apache.isis.core.metamodel.spec.ManagedObject;
+import org.apache.isis.core.metamodel.spec.ObjectSpecification;
+
+import lombok.NonNull;
+import lombok.val;
+import lombok.extern.log4j.Log4j2;
+
+@Log4j2
+public class JpaEntityFacet
+ extends FacetAbstract
+ implements EntityFacet {
+
+ private final Class<?> entityClass;
+ private final ServiceRegistry serviceRegistry;
+ private final List<IdStringifier<?>> idStringifiers;
Review Comment:
could use a `Can` instead of `List` (so no need to convert on assignment)
(remember, `Can` has clear semantics of being immutable)
##########
persistence/jdo/datanucleus/src/main/java/org/apache/isis/persistence/jdo/datanucleus/metamodel/facets/entity/JdoEntityFacet.java:
##########
@@ -80,6 +81,7 @@
@Inject private ObjectManager objectManager;
@Inject private ExceptionRecognizerService exceptionRecognizerService;
@Inject private JdoFacetContext jdoFacetContext;
+ @Inject private List<IdStringifier> idStringifiers;
Review Comment:
might be irrelevant, but probably use
```java
@Autowired(required=false) private List<IdStringifier> idStringifiers;
```
just to be safe when JUnit testing, and no IdStringifiers are registered
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
