Switch grails-datastore-gorm-test back to api so SimpleMapDatastore can be found in test

[jdaugherty]

No description provided.

[jdaugherty]

I'm doing cleanup in downstream projects like grails-cache and noticed that they are failing to compile. It's because the SimpleMapDatstore is no longer being exported like it was before. I'm going to go ahead and merge this change since it's a revert of an earlier change. @matrei please let me know if you disagree with this and we can discuss.

[matrei]

@jdaugherty Hmm 🤔, why don't we add grails-datastore-gorm-test as a direct dependency to the projects that depend on it?

Or should perhaps grails-datastore-gorm-test be merged into grails-gorm-testing-support now that grails-gorm-testing-support is part of the grails-data-mapping project. I am not familiar with grails-datastore-gorm-test. By the name of it, it sounds like it belongs in grails-gorm-testing-support. Is it used outside tests?

[jdaugherty]

I think the idea of it being separate is people can include it as a simple datastore implementation if they don't want a full datastore implementation. Otherwise, it would have been merged into the testing support library.

It's reasonable to subdivide a project so it's used by default in test, and can be used by others outside of test without the test code.

[jdaugherty]

I also disagree on adding it as a direct dependency since historically it's always been included. It's actually required for unit tests, so I don't see a case where we wouldn't always include it with test - so it should stay api.

[matrei]

I don't think it is required as a compile time dependency for unit tests. In that case we would have noticed failures everywhere. However, it is being included as a runtime dependency by being implementation.

[codeconsole]

I actually use grails-datastore-gorm-test as a gorm implementation for in memory apps. It is dramatically more light weight than using a h2 in memory database with a hibernate stack. It is a great tool to build sample apps off of as well. @matrei grails-datastore-gorm-test is a simple map gorm implementation. It used to be named that, but it was renamed.

[jdaugherty]

Have we actually confirmed it's working for Domain Unit tests? I suspect it's not given that they were failing to compile in the grails cache project. i don't see any functional tests that demonstrate this usage inside this project. Changing to api fixed those tests.

[matrei]

Have we actually confirmed it's working for Domain Unit tests? I suspect it's not given that they were failing to compile in the grails cache project

If you look in the grails-functional-tests project, there is not a single mention of the grails-datastore-gorm-test library and there are some DomainUnitTests, working just fine after the scope change of the grails-datastore-gorm-test in grails-gorm-testing-support.
They are working, because grails-datastore-gorm-test is available on the testRuntimeClasspath, brought in transitively by grails-gorm-testing-support.

grails-cache (CacheableParseSpec) fails to compile, because CacheableParseSpec is directly depending on (using classes from) grails-datastore-gorm-test.

From my point of view, with the info I have now, the correct thing to do is rename grails-datastore-gorm-test to grails-datastore-gorm-simple and have it as an implementation scoped dependency in grails-gorm-testing-support. grails-cache should add grails-datastore-gorm-simple as a testImplementation dependency as a test class is directly referencing SimpleMapDatastore.

[jdaugherty]

Concerning the naming, I agree we should rename it so it's clear on the purpose of the library.

Concerning the combining, because it can be used for other purposes (and even has per Scott), we shouldn't combine it.

Concerning the dependency split, I don't agree for multiple reasons:

1. Unit tests are only working because of the transitive exposure. If it's used by the library on the runtime classpath, why can't it be used on the compile classpath by default for simplicity?

2. There is only 1 possible implementation that can be used in unit tests, and this is it. If there were more, I'd agree it could be separate. By requiring people to add more dependencies, we're requiring people to add even more dependencies to build.gradle and understand what those are, including the defaults. If we were to split this, it would make sense if was something people had to define which implementation to use, but this is always going to have to be defined as this one.

3. We don't take the same approach with data mapping - when you include:

   implementation "org.grails.plugins:hibernate5"
   

It pulls in the data mapping projects because it's the only option. The same should apply here.

[matrei]

Concerning the naming, I agree we should rename it so it's clear on the purpose of the library.

👍

Concerning the combining, because it can be used for other purposes (and even has per Scott), we shouldn't combine it.

Agree, there are use cases outside testing.

Unit tests are only working because of the transitive exposure. If it's used by the library on the runtime classpath, why can't it be used on the compile classpath by default for simplicity?

This is why there are different scopes to begin with. Not every transitive dependency of a library should be added to the compileClasspath. Only those transitive dependencies which are exposed in the public api of the library. This is what the api scope is for.

There is only 1 possible implementation that can be used in unit tests, and this is it.

And, in my view, it is an implementation detail that should not be leaked to the compile classpath of consumers.

By requiring people to add more dependencies

We are only requiring people to add dependencies they are directly depending on (which is a good thing). If we want to, we can create "dependency aggregators (starters)" for the app level to simplify (hide) dependency management for end users, but I think the dependency chains at at the framework level should be correct.

We don't take the same approach with data mapping

Actually, I think we do. org.grails:grails-datastore-gorm is an actual api dependency (used in the public api) of org.grails.plugins:hibernate5 classes. Now, the dependencies in org.grails.plugins:hibernate5 has not yet been cleaned up, so there may be dependency changes, but this fact will not change.

Now, one could argue that Grails Plugins could/should act as aggregators, and some actually do. Like org.grails.plugins:mongodb adds grails-datastore-gorm-mongodb-ext even though this is not a dependency. But these are rare and special cases, like adding Groovy Extensions.

[jdaugherty]

Unit tests are only working because of the transitive exposure. If it's used by the library on the runtime classpath, why can't it be used on the compile classpath by default for simplicity?

This is why there are different scopes to begin with. Not every transitive dependency of a library should be added to the compileClasspath. Only those transitive dependencies which are exposed in the public api of the library. This is what the api scope is for.

There is only 1 possible implementation that can be used in unit tests, and this is it.

I consider this part of the testing public api though. If you want to check the state it ended up in the datastore, you have to inject it. Which means if you're typing, you'll want that class on your classpath.

And, in my view, it is an implementation detail that should not be leaked to the compile classpath of consumers.

I don't view it as an implementation detail since it's being used in tests.

By requiring people to add more dependencies

We are only requiring people to add dependencies they are directly depending on (which is a good thing). If we want to, we can create "dependency aggregators (starters)" for the app level to simplify (hide) dependency management for end users, but I think the dependency chains at at the framework level should be correct.

We don't take the same approach with data mapping

I'd argue with testing this should be an aggregator. the gorm-testing-support library is everything related to testing gorm.

Actually, I think we do. org.grails:grails-datastore-gorm is an actual api dependency (used in the public api) of org.grails.plugins:hibernate5 classes. Now, the dependencies in org.grails.plugins:hibernate5 has not yet been cleaned up, so there may be dependency changes, but this fact will not change.

Now, one could argue that Grails Plugins could/should act as aggregators, and some actually do. Like org.grails.plugins:mongodb adds grails-datastore-gorm-mongodb-ext even though this is not a dependency. But these are rare and special cases, like adding Groovy Extensions.

I consider this one of those cases. Normally this library would have been apart of the testing library and naturally exposed by being in the same library. The only reason it's not is so other people can use it outside of test.

[matrei]

Looking at CacheableParseSpec, it is not a DomainUnitTest. That's why it needs to setup it's own datastore.
CacheableParseSpec does not need grails-gorm-testing-support.

If you want to check the state it ended up in the datastore, you have to inject it. Which means if you're typing, you'll want that class on your classpath.

I don't understand how you mean here. Could you show me a use case, maybe I'll grok why you want this change.

I'd argue with testing this should be an aggregator. the gorm-testing-support library is everything related to testing gorm.

I'm not against setting up aggregators. But libraries are not aggregators. I think aggregators should only aggregate.
With this change unnecessary classes are forced onto consumers testCompileClasspath.