Expose PersistentResource's setId Logic

[jstemen]

While perusing the Elide code base I noticed two bits of code that look pretty similar:

https://github.com/yahoo/elide/blob/master/elide-core/src/main/java/com/yahoo/elide/core/datastore/inmemory/InMemoryTransaction.java#L134

And

https://github.com/yahoo/elide/blob/master/elide-core/src/main/java/com/yahoo/elide/core/PersistentResource.java#L751

I'm in the middle of implementing my own custom datastore, and so far, I've had to copy the setId logic from the InMemoryTransaction class, so now very similar,reflection-based code is copied across 3 places. It seems like implementing a new DataStore would be a lot easier if the following was done:

1. Change the DataStoreTransaction interface's method signatures from working with a generic Object entity to work with a PersistentResource entity. Link to the current interface here: https://github.com/yahoo/elide/blob/master/elide-core/src/main/java/com/yahoo/elide/core/DataStoreTransaction.java#L83

2)Add setId() to com.yahoo.elide.security.PersistentResource so that it can be used outside of com.yahoo.elide.core.PersistentResource

Ultimately, it would be nice if the InMemoryTransaction could look like this:

    @Override
// ------------> I changed this line
    public void createObject(PersisentResource entity, RequestScope scope) {
        Class entityClass = entity.getClass();

        // TODO: Id's are not necessarily numeric.
        AtomicLong nextId;
        synchronized (dataStore) {
            nextId = typeIds.computeIfAbsent(entityClass,
                    (key) -> {
                        long maxId = dataStore.get(key).keySet().stream()
                                .mapToLong(Long::parseLong)
                                .max()
                                .orElse(0);
                        return new AtomicLong(maxId + 1);
                    });
        }
        String id = String.valueOf(nextId.getAndIncrement());
// ------------> I changed this line
        entity.setId(id);
        operations.add(new Operation(id, entity, entity.getClass(), false));
    }

In the above code, I've replaced the InMemoryTransaction.setId method with the PersisentResource's.

Alternatively, if people don't want to expose more of the PersisentResource's internal functions, we could extract the reflection based bits into a seperate worker class of some sort that could be used by both the PersisentResource and also Transaction implementations.

[aklish]

@jstemen Thanks for filing the issue. I agree that some refactoring is in order here - especially providing a convenient mechanism to set an ID since that is one of the core things a DataStore does for every object created (that has not had an ID provided externally).

A few thoughts:

1. In general, nearly all of the reflection in Elide is done in the EntityDictionary. It already has a method called getId: https://github.com/yahoo/elide/blob/master/elide-core/src/main/java/com/yahoo/elide/core/EntityDictionary.java#L890-L909

My initial thought is that this capability might belong best in the EntityDictionary.

2. Although not explicitly documented anywhere, we have some tribal knowledge about what we consider to be public interfaces/classes and what is not. Changes to the public interfaces would correspond to a major release. DataStore and DataStoreTransaction are currently in that bucket. However, PersistentResource is not in that bucket - and I'm not sure we want to put it into that bucket of public concepts. It would need a pretty big refactor/cleanup in its current state.

3. If we move this code into EntityDictionary, we shouldn't need to spin a major revision. If you are open to submitting a PR, I would be happy to review. I think you've called out a definite pain point and something that should be improved.

Thanks again.