Isis 1976 rethink object adapters #120

andi-huber · 2018-09-04T08:54:47Z

Key changes so far ...

Interface AdapterManger which allowed read/write access to the ObjectAdapter - cache/map is now gone.

This is replaced by new interface ObjectAdapterProvider, which does not expose the (if any) ObjectAdapter - cache/map.

This affects a lot of facet classes within the metamodel.

andi-huber · 2018-09-04T09:29:01Z

core/runtime/src/main/java/org/apache/isis/core/runtime/persistence/adapter/PojoAdapter.java

    @Override
    public Oid getOid() {
        return oid;
    }

-    @Override
-    public void replaceOid(Oid persistedOid) {


field oid in ObjectAdapter is now final
oid replacement no longer allowed, so we intoduced ObjectAdapter withOid(RootOid newOid) to create a copy

+1 This looks like it's the main significant change in this PR... with the replaceOid stuff now removed.

andi-huber · 2018-09-04T09:36:21Z

.../src/main/java/org/apache/isis/core/runtime/sessiontemplate/AbstractIsisSessionTemplate.java

-    protected final ObjectAdapter adapterFor(final RootOid rootOid) {
-        return getPersistenceSession().adapterFor(rootOid);
-    }
+//TODO[ISIS-1976] not used !?


possibly changes behavior, need to investigate

AbstractIsisSessionTemplate is public API, so while it might not be used within the framework, it's possible that it's used by application code. However, let's remove it for now, see if we can simplify. If we do need an API like this, it should probably take Bookmark (from the applib) rather than RootOid. Also, should probably return just an Object (the domain object) rather than an ObjectAdapter - ie let's keep ObjectAdapter as an internal implementation detail and not in any of our APIs.

andi-huber · 2018-09-04T09:38:27Z

...ava/org/apache/isis/core/runtime/system/persistence/adaptermanager/ObjectAdapterContext.java

+ *  
+ * @since 2.0.0-M2
+ */
+public class ObjectAdapterContext {


class is a product of refactoring, needs refinement later on; but is sane so far

andi-huber · 2018-09-04T09:40:31Z

...ava/org/apache/isis/core/runtime/system/persistence/adaptermanager/ObjectAdapterContext.java

+        return mementoSupportMixin;
+    }
+
+    // ------------------------------------------------------------------------------------------------


from here downward: code is work in progress

andi-huber · 2018-09-04T09:42:30Z

...isis/core/runtime/system/persistence/adaptermanager/ObjectAdapterContext_AdapterManager.java

+ *  
+ * @since 2.0.0-M2
+ */
+class ObjectAdapterContext_AdapterManager {


package private mixin for ObjectAdapterContext: responsibility AdapterManager 'legacy' support

andi-huber · 2018-09-04T09:43:07Z

...he/isis/core/runtime/system/persistence/adaptermanager/ObjectAdapterContext_Consistency.java

+ *  
+ * @since 2.0.0-M2
+ */
+class ObjectAdapterContext_Consistency {


package private mixin for ObjectAdapterContext: responsibility Cache/Map consistency

andi-huber · 2018-09-04T09:43:43Z

...ache/isis/core/runtime/system/persistence/adaptermanager/ObjectAdapterContext_Factories.java

+ *  
+ * @since 2.0.0-M2
+ */
+class ObjectAdapterContext_Factories implements ObjectAdapterFactories {


package private mixin for ObjectAdapterContext: responsibility provides ObjectAdapter factories

andi-huber · 2018-09-04T09:44:39Z

...isis/core/runtime/system/persistence/adaptermanager/ObjectAdapterContext_MementoSupport.java

+ *  
+ * @since 2.0.0-M2
+ */
+class ObjectAdapterContext_MementoSupport implements MementoRecreateObjectSupport {


package private mixin for ObjectAdapterContext: responsibility provide object recreation to mementos

andi-huber · 2018-09-04T09:45:53Z

...re/runtime/system/persistence/adaptermanager/ObjectAdapterContext_ObjectAdapterProvider.java

+ *  
+ * @since 2.0.0-M2
+ */
+class ObjectAdapterContext_ObjectAdapterProvider implements ObjectAdapterProvider {


package private mixin for ObjectAdapterContext: responsibility provides ObjectAdapterProvider implementation

...java/org/apache/isis/core/runtime/system/persistence/adaptermanager/ObjectAdapterLegacy.java

.../java/org/apache/isis/viewer/wicket/viewer/integration/wicket/ConverterForObjectAdapter.java

andi-huber · 2018-09-04T09:48:58Z

...rg/apache/isis/viewer/wicket/viewer/integration/wicket/ConverterForObjectAdapterMemento.java

        final Oid oid = RootOid.deStringEncoded(value);
-        final ObjectAdapter adapter = getPersistenceSession().getAdapterFor(oid);
-        return ObjectAdapterMemento.createOrNull(adapter);
+


Method appears not used to me, but not really sure!

Not sure either.

IConverter is part of Wicket's SPI, to convert strings that are input in forms to underlying objects. So this one seems to convert a string representation of an object (eg "cust.Customer:123", like a Bookmark#toString() would return) into an ObjectAdapterMemento, which is basically the Wicket UI's serializable model for an ObjectAdapter (more or less the same as a RootOid).

So, my guess is that it isn't used, because I can't think of anywhere that we have this abiliity for a user to enter "cust.Customer:123" and have the framework look up the corresponding object reference.

Therefore, let's not worry about it too much. Perhaps add a TODO to see about removing this entire class.

ObjectAdapter caching Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

adapterFor(RootOid)

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

introduces ObjectAdapterProvider Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ObjcetAdapterProvider from each other Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

PersistenceSession to ObjectAdapterContext Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ObjectAdapterProvider introduces ObjectAdapterProvider.Delegating removes AdapterManager ports changes from DN5-plugin to DN-4 plugin Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ObjectAdapterContext.specificationForViewModel() Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

andi-huber · 2018-09-04T20:36:10Z

...ava/org/apache/isis/core/metamodel/facets/collections/javautilcollection/JavaArrayFacet.java

-            final ObjectAdapter element = getAdapterManager().getAdapterFor(element2);
-            objectCollection.add(element);
+        for (final Object element : array) {
+            final ObjectAdapter elementAdapter = getObjectAdapterProvider().adapterFor(element);


change in behavior:
BEFORE: only a map lookup, which would not trigger ObjectAdapter creation
AFTER: triggers ObjectAdapter creation if required

I think this is ok.

I presume that if element == null then this implies that elementAdapter == null ?

Yes, pojo is nullable in ObjectAdapterContext_ObjectAdapterProvider ...

public ObjectAdapter adapterFor(Object pojo) { if(pojo == null) { return null; } ... }

andi-huber · 2018-09-04T20:37:14Z

...ava/org/apache/isis/core/metamodel/facets/collections/javautilcollection/JavaArrayFacet.java

+        if(isEmpty(array)) {
+            return null;
+        }
+        return array.length > 0 ? getObjectAdapterProvider().adapterFor(array[0]) : null;


change in behavior:
BEFORE: only a map lookup, which would not trigger ObjectAdapter creation
AFTER: triggers ObjectAdapter creation if required

I think this is ok.

andi-huber · 2018-09-04T20:37:50Z

...rg/apache/isis/core/metamodel/facets/collections/javautilcollection/JavaCollectionFacet.java

        // or if a copy is sufficient
-        return Collections2.transform(collectionOfUnderlying,
-                ObjectAdapter.Functions.adapter_ForUsing(getAdapterManager()));
+        return Collections2.transform(pojoCollection,


no change in behavior, but ...
Is this live-view actually required?

It's probably would be fine to be a copy, but is there any need to change the behaviour?

no need to change behavior yet; just a standing question, that prevents removal of guava's class Collections2

danhaywood · 2018-09-05T05:53:41Z

...ava/org/apache/isis/core/runtime/system/persistence/adaptermanager/ObjectAdapterContext.java

+        }
+    }
+
+    // -- CACHING


I see we do still have our caches, for the moment at least.

yes, the point of merging this current state is to reduce the scope of java classes that are involved with coming changes with ISIS-1976

danhaywood · 2018-09-05T06:00:01Z

core/metamodel/src/main/java/org/apache/isis/core/metamodel/adapter/ObjectAdapterProvider.java

+     * FIXME[ISIS-1976] Note: whether or not 'bypassing mapping' should not be exposed by the API.
+     * So this further needs refactoring. 
+     */
+    ObjectAdapter specificationForViewModel(final Object viewModelPojo);


This method name seems strange, as compared to the "adapterForViewModel" method below, because it returns an ObjectAdapter, not an ObjectSpecification.

My guess of what you're doing here is that the returned ObjectAdapter is ok to call "getSpecification()" on, but is in some sense incomplete because it has bypassed mapping?

Having these "incomplete" ObjectAdapters around seems dangerous though. Your comment does indeed have a FIXME. Is this code used elsewhere? What calls it?

The original code, had me fooled, because it exploited a side-effect. So I had to quick fix this late last night.
2c70410

The interface for ObjectAdapterProvider will definitely change with further refactoring step.

danhaywood · 2018-09-05T06:01:11Z

I think this PR is fine to merge.

To me it seems that mostly this is just moving functionality about ... the OidAdapterMap and PojoAdapterMap are still present, but relocated to ObjectAdapterContext.

The most significant change is that ObjectAdapter#rootOid is now final, and so the replacePojo stuff has gone, instead the ObjectAdapter is copied if the rootOid changes. This seems ok to me - or at least will be once we get to the stage where ObjectAdapters are instantiated as required and then discarded immediately afterwards.

The ObjectAdapterProvider#specificationForViewModel(...) - which returns an ObjectAdapter - looks a bit odd; I've created a separate comment for that. I suspect this is work in progress so happy to see how it plays out, but this code does look weird at the moment.

danhaywood

OK to merge, though I've added some comments above and elsewhere on the PR.

Main query re: the naming and responsibiliities of ObjectAdapterContext#specificationForViewModel(...)

danhaywood · 2018-09-05T06:05:20Z

...me/src/main/java/org/apache/isis/core/runtime/system/persistence/PersistenceSessionBase.java

        this.transactionManager = new IsisTransactionManager(this, /*authenticationSession,*/ servicesInjector);

        this.state = State.NOT_INITIALIZED;
-


Did this configuration property get removed? Or was the code just moved elsewhere? I'd prefer to keep it than lose it.

regarding property 'concurrencyCheckingGloballyEnabled': code moved here

https://github.com/apache/isis/blob/2c7041016faf87443befaa80eef9a0457ac1338f/core/metamodel/src/main/java/org/apache/isis/core/metamodel/adapter/concurrency/ConcurrencyChecking.java#L32

andi-huber requested a review from danhaywood September 4, 2018 08:56

andi-huber commented Sep 4, 2018

View reviewed changes

...java/org/apache/isis/core/runtime/system/persistence/adaptermanager/ObjectAdapterLegacy.java Show resolved Hide resolved

andi-huber commented Sep 4, 2018

View reviewed changes

.../java/org/apache/isis/viewer/wicket/viewer/integration/wicket/ConverterForObjectAdapter.java Show resolved Hide resolved

andi-huber commented Sep 4, 2018

View reviewed changes

andi-huber and others added 17 commits September 4, 2018 20:57

ISIS-1976: add layer of abstraction as first step to deprecate

9d64857

ObjectAdapter caching Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: experimental refactoring

1a7bb26

adapterFor(RootOid)

ISIS-1976: make ObjectAdapter's Oid final

d43ad07

ISIS-1976: minor simplification

a0e04a1

ISIS-1976: make RootAndCollectionAdapters package private

6e21d73

ISIS-1976: add some comments

791fc65

ISIS-1976: decouple PojoAdapter from DN

246db19

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: decouple ObjectAdapter creation from PersistenceSession

ccb1571

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: remove AdapterManagerBase

4130493

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: rename some methods in AdapterManager

62e3128

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: rename IsisLifeCycleListener2 -> IsisLifeCycleListener

29fbfff

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: minor simplifications, also reduce compiler warnings

5e59bd3

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: decouple metamodel's facets from AdapterManager

3812bc1

introduces ObjectAdapterProvider Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: add missing license header

663cbd5

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: further decouple factets

48e4be1

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: remove unused spec-loader from some facet constructors

83f6173

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: further decouple from AdapterManager

ee9ce0f

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

andi-huber added 7 commits September 4, 2018 20:57

ISIS-1976: let PersistenceSession no longer implement AdapterManager

9ef889f

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: decouple implementations of PersistenceSession and

a7ccf6a

ObjcetAdapterProvider from each other Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: move responsibility for pre-loading service adapters from

1cf124b

PersistenceSession to ObjectAdapterContext Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: PersistentSession: remove virtually unreachable code

93ac9f2

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: moving responsibilities from PersistenceSession to

7770404

ObjectAdapterProvider introduces ObjectAdapterProvider.Delegating removes AdapterManager ports changes from DN5-plugin to DN-4 plugin Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: minor cleanup

6955272

Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

ISIS-1976: fixes StackOverflow with

2c70410

ObjectAdapterContext.specificationForViewModel() Task-Url: https://issues.apache.org/jira/browse/ISIS-1976

andi-huber force-pushed the ISIS-1976-rethink-object-adapters branch from 074cecf to 2c70410 Compare September 4, 2018 20:25

andi-huber commented Sep 4, 2018

View reviewed changes

danhaywood reviewed Sep 5, 2018

View reviewed changes

danhaywood approved these changes Sep 5, 2018

View reviewed changes

danhaywood reviewed Sep 5, 2018

View reviewed changes

andi-huber merged commit 3d49fc7 into master Sep 5, 2018

		this.transactionManager = new IsisTransactionManager(this, /authenticationSession,/ servicesInjector);

		this.state = State.NOT_INITIALIZED;

Isis 1976 rethink object adapters #120

Isis 1976 rethink object adapters #120

Uh oh!

Conversation

andi-huber commented Sep 4, 2018

Uh oh!

andi-huber Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danhaywood commented Sep 5, 2018

Uh oh!

danhaywood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andi-huber Sep 4, 2018 •

edited

Loading

danhaywood left a comment •

edited

Loading