Fix Session.load/queryForObject nullability

[TWiStErRob]

Description

Add tests and fix nullability for these methods that can and will return null.

Related Issue

See first commit for failing tests for repro.

Motivation and Context

The mentioned method explicitly declare nullability, but they're restricted in the Kotlin extension functions.

neo4j-ogm/core/src/main/java/org/neo4j/ogm/session/Session.java

Lines 489 to 494 in 0fafd9b

	/**
	* Load single entity instance of type, with default depth = 1
	*
	* @return entity instance, null if not found
	*/
	<T, ID extends Serializable> T load(Class<T> type, ID id);

neo4j-ogm/core/src/main/java/org/neo4j/ogm/session/Session.java

Lines 496 to 501 in 0fafd9b

	/**
	* Load single entity instance of type, with depth
	*
	* @return entity instance, null if not found
	*/
	<T, ID extends Serializable> T load(Class<T> type, ID id, int depth);

neo4j-ogm/core/src/main/java/org/neo4j/ogm/session/Session.java

Lines 609 to 613 in 0fafd9b

	* @return An instance of the objectType that matches the cypher and parameters. Null if no object
	* is matched
	* @throws java.lang.RuntimeException If more than one object is found.
	*/
	<T> T queryForObject(Class<T> objectType, String cypher, Map<String, ?> parameters);

This causes a problem when used like this:

import org.neo4j.ogm.session.queryForObject

class UserService(
	private val session: Session
) {

	fun find(id: String): User =
		session.queryForObject(
			"""
			MATCH (u:User{id:{id}})
			RETURN u AS user
			""",
			mapOf(
				"id" to id
			)
		)
}

Notice that because queryForObject declared a non-null return type, I can too. But when I call find with a wrong ID, it'll blow up with:

java.lang.IllegalStateException: queryForObject(T::class.java, cypher, parameters) must not be null

This means that a simple inline utility function changes behavior of the method on the interface.

Note that T is restricted to non-null (: Any) in generics. Adding T? as return value is explicitly making this nullable, which it should be. Alternatively changing to <T: Any?> would work too, but it's not safe as seen here:

notice how specifying the type argument for the function (explicit or inferred doesn't matter), we're telling Kotlin one thing, but receiving another. See how a non-null inferred result is actually passing an assertNull test?! 😨 If it was actually using that variable, it would blow up with NPE.

The big problem here is that in Kotlin a change like this is like changing from an interface to a concrete type in Java. It's going to break practically everyone who used this method. The fix is easy though: either !! if they're sure the value they're querying is there, or add a null check (if/?./?: fallback/?: return/?: error()) to signal appropriately for their project.

How Has This Been Tested?

Added unit tests.
TODO: should I add an integration test too for not found value being null?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  It is a breaking change because every time someone used this function they assumed non-null, see context above.

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
  As in: a big warning in Changelog and release notes.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[michael-simons]

Good catch and bummer on my side :/ I'm gonna deal with the warning in the readme and changelog.

But I think it's much better to annoy people with a breaking change in the API in a patch release than having issues around nullability in Kotlin. WDYT, @meistermeier ?

[TWiStErRob]

I forgot to mention that SessionFactory.unwrap has the same problem, theoretically unwrap can be null:

neo4j-ogm/embedded-driver/src/main/java/org/neo4j/ogm/drivers/embedded/driver/EmbeddedDriver.java

Lines 181 to 188 in 0fafd9b

	public <T> T unwrap(Class<T> clazz) {
	if (clazz == GraphDatabaseService.class) {
	return (T) graphDatabaseService;
	} else {
	return super.unwrap(clazz);
	}
	}

if the driver is closed:

neo4j-ogm/embedded-driver/src/main/java/org/neo4j/ogm/drivers/embedded/driver/EmbeddedDriver.java

Lines 172 to 179 in 0fafd9b

	public synchronized void close() {
	if (graphDatabaseService != null) {
	logger.info("Shutting down Embedded driver {} ", graphDatabaseService);
	graphDatabaseService.shutdown();
	graphDatabaseService = null;
	}
	}

But I think that may need a different fix, maybe make sure EmbeddedDriver doesn't return null?
(BoltDriver will initialize itself, it'll never be null)
Shall I open a bug for this to figure out?

[michael-simons]

I think it's ok to change the unwrap extension to return possible null as well. I'll do this.

I'd rather see bolt and embedded driver behave the same I think: So either return null on the bolt driver when it's not initialised instead of initialising from that call. Gonna think about it…