HHH-19849 Add an SPI that allows attaching session-scoped "extensions" to the session/statelesssession implementors

[marko-bekhta]

https://hibernate.atlassian.net/browse/HHH-19849

so that Search wouldn't need to rely on .getProperties() and abuse it 😄.

I'm not "attached" 🙂 to the method names and if there are any suggestions for better alternatives, happy to change them 🙂

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

[yrodiere]

LGTM, though we definitely need Steve's opinion here.

[sebersole]

Perhaps this is over-thinking, but what do you think about something like this instead:

/** For now, just a marker */
interface Extension {
}

interface ExtensionStorage {
    ...
}

interface SharedSessionContractImplementor {
    ...
    ExtensionStorage getExtensionStorage(Class<? extends Extension> extension);
}

[yrodiere]

Perhaps this is over-thinking, but what do you think about something like this instead

It looks good, but on the other hand I'm not sure ExtensionStorage would provide any useful feature to Hibernate Search. Hibernate Search really only needs to bind its SearchSession to the Session.

Perhaps you were thinking of something specific Envers could leverage?

[gavinking]

Let's avoid stringly-typed extensionNames. I think that's what Steve is suggesting.

[marko-bekhta]

Perhaps this is over-thinking, but what do you think about something like this instead:

/** For now, just a marker */
interface Extension {
}

interface ExtensionStorage {
    ...
}

interface SharedSessionContractImplementor {
    ...
    ExtensionStorage getExtensionStorage(Class<? extends Extension> extension);
}

Thanks, that might work 🙂. Just made an adjustment to the pr 🙂

In Search, we have a session and a session holder, we connect the holder to the ORM's session.
With that in mind, I could see how we'd make the session holder an ExtensionStorage. I couldn't think of how we'd use Extension on the Search side in this case, so I didn't add it here.

[yrodiere]

I just heard GraalVM cry a little. Do we really need this?

[marko-bekhta]

🫣 well... not really, at least not for Search 🙂.

[yrodiere]

Then let's not! SPI users can figure it out :)

[yrodiere]

Suggested change

	Object storage = extensionStorages.get( extension );;
	Object storage = extensionStorages.get( extension );

[yrodiere]

Suggested change

	@Message("Failed to create a storage of type %s: %s")
	@Message("Failed to create extension storage of type %s: %s")

[mbellade]

Perf tip:

Suggested change

	Object storage = extensionStorages.get( extension );
	if ( storage == null ) {
	storage = createIfMissing.get();
	extensionStorages.put( extension, storage );
	}
	Object storage = extensionStorages.computeIfAbsent( extension, k -> supplier.get() );

[yrodiere]

Really, this performs better?

That's the code I would use by default (because it's obviously cleaner) but I've had people complain that it doesn't perform well due to the extra lambda.

[marko-bekhta]

yeah... I didn't want to create unnecessary lambdas, but if you are saying this should perform better 👍🏻 🙂 thanks!

[marko-bekhta]

ha 🙂, now @mbellade we need to benchmark it 😁

[marko-bekhta]

so I got curious how much that lambda would hurt ...

SimpleTest.computeIfAbsent  thrpt     56198.917 ± 254.380  ops/ms
SimpleTest.get              thrpt     77517.446 ± 497.622  ops/ms

I'll keep the get for now 🫣 🙂

[mbellade]

I'm guessing this was run with the same key every time, such that put is only invoked once. The instantiation cost of the lambda of course will have an impact, but accessing an hash-map twice (one get and one put) should be much worse if the entry doesn't exist yet.

So it really depends on the use-case :)

Edit: if there wasn't a Supplier but an Object instead, putIfAbsent would be the winner regardless of the use case.

[sebersole]

Perhaps this is over-thinking, but what do you think about something like this instead

It looks good, but on the other hand I'm not sure ExtensionStorage would provide any useful feature to Hibernate Search. Hibernate Search really only needs to bind its SearchSession to the Session.

Perhaps you were thinking of something specific Envers could leverage?

Here specifically I was thinking about avoiding String keys for a few reasons.

But I was thinking also that we've spoken a few times about "true extensions" (for lack of a better phrase) for other projects to expose via Session (Envers reader e.g.). So partially I was thinking about that. So maybe even something like this:

interface SharedSessionContractImplementor {
    ...
    <E extends Extension> E getExtensionStorage(Class<E> extensionType);
}

which would rely on projects registering this mechanism with ORM. E.g. for Search:

class SearchExtension implements Extension {
    // access to storage
}

SearchExtension ext = session.getExtension( SearchExtension.class );
ext.storeSomething( ... );

For envers:

class EnversExtension implements Extension {
    AuditReader getAssociatedAuditReader();
    ...
}
...

Just brainstorming Marko, no need to make changes yet :)

I know this is well beyond the scope of the issue here, but I think its worth considering.

[yrodiere]

Just brainstorming Marko, no need to make changes yet :)

I think Marko already did pretty much what you're suggesting here, give or take a few names 😅

[sebersole]

Yes and no. I'm actually I am thinking something akin to Gradle's notion of extensions.

interface Extension {
}

/** Registered with SessionFactory */
interface ExtensionIntegration<E extends Extension> {
    Class<E> getExtensionType();
    E createExtension(SharedSessionContractImplementor session, ???);
}

interface SharedSessionContractImplementor {
    ...

    <E extends Extension> E getExtension(Class<E> extensionType);
}