Introduce a dedicated `ProjectClient` interface #132237

nielsbauman · 2025-07-31T06:45:08Z

A dedicated ProjectClient interface will make it easier to guarantee a project scope on a type/compilation basis.

A dedicated `ProjectClient` class will make it easier to guarantee a project scope on a type/compilation basis.

server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java

nielsbauman · 2025-07-31T06:51:07Z

server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java

+            // We construct a dedicated project client for the default project if we're in single project mode. This dedicated project
+            // client is an optimization in that it does not use the project resolver and instead executes the request directly.
+            if (defaultProjectClient == null) {
+                defaultProjectClient = new ProjectClient(this, projectId) {


I wanted to avoid introducing a performance regression in single-project mode by having to create a new client on every invocation, so I went for a non-final field. Other suggestions are welcome too.

I think we can also pro-actively create it at instantiation time similar to AdminClient. In a non-MP setup, it is guaranteed that some code will need it.

I remember why I had to construct the default project client lazily: because constructing it in the constructor results in a cyclic dependency/stack overflow.

Is it still a problem if we create defaultProjectClient at the end of the constructor? I'd prefer to have the field being final if possible.

The issue is that we're constructing the ProjectClient inside the constructor of AbstractClient, but ProjectClient itself also extends AbstractClient. So the place inside the constructor doesn't matter much - if I understand your suggestion correct.

I just realized that we could probably construct the default project client conditionally, only if this is not an instance of ProjectClient (as we'll never need a default project client in that case anyway). What do you think of that solution?

Hm, calling projectResolver.supportsMultiProjects() in the constructor is making a lot of tests fail because they use TestProjectResolvers.alwaysThrow(). TBH, I'm more inclined to remove the supportsMultiProjects() call instead of updating all those tests. What do you think?

I'd prefer updating tests instead of bending production code for them unless there is more unexpected complexity? It's a redundant field in MP and it makes sense to be null. Or I guess someone would probably try to refactor it that way in future.

Hm ok, and I assume you're also no fan of changing supportsMultiProjects() in TestProjectResolvers.alwaysThrow() to return false instead of throwing? I'm just trying to find a way to avoid having to change dozens of test classes.

Yep unless we rename alwaysThrow to something else to reflect its new behaviour. To help with progress here, we can drop the projectResolver.supportsMultiProjects check in the constructor in this PR and get back to it in a follow-up? What do you think?

That makes sense to me 👍. Done in f371275.

nielsbauman · 2025-07-31T06:52:32Z

x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MlIndexTemplateRegistryTests.java

-        when(client.projectClient(any())).thenReturn(client);
-        AdminClient adminClient = mock(AdminClient.class);
-        IndicesAdminClient indicesAdminClient = mock(IndicesAdminClient.class);
-        when(adminClient.indices()).thenReturn(indicesAdminClient);
-        when(client.admin()).thenReturn(adminClient);
-        doAnswer(withResponse(AcknowledgedResponse.TRUE)).when(indicesAdminClient).putTemplate(any(), any());


These lines weren't necessary. The same goes for the watcher test class below.

elasticsearchmachine · 2025-07-31T06:53:30Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

ywangd

I have a question about class vs interface.

ywangd · 2025-07-31T07:10:20Z

server/src/main/java/org/elasticsearch/client/internal/ProjectClient.java

+ * A dedicated {@link Client} that is scoped to a specific project. It will set the project ID in the thread context before executing any
+ * requests.
+ */
+public class ProjectClient extends FilterClient {


I had thought the ProjectClient would be an interface rather than a concrete class, e.g. something like

public interface ProjectClient extends Client { ProjectId getProjectId(); }

Having a concrete ProjectClient class that extends FilterClient feels a bit odd to me since they are conceptually different things. Having the FilterClient as the implementation and returned from the projectClient() method makes more sense to me.

Having a concrete ProjectClient class that extends FilterClient feels a bit odd to me since they are conceptually different things.

Why are they conceptually different things to you? All that a ProjectClient does is add the project ID in the thread context, which is exactly what a FilterClient is for. ProjectClient doesn't need to extend FilterClient. It can extend AbstractClient as well, but extending FilterClient reduced a little bit of boilerplate code. Can you explain why you think they are conceptually different but you do think using a FilterClient at runtime does make sense?

IMO, a ProjectClient interface like that doesn't really dictate anything about the client. The only method it provides is the getter. Any usages of the interface will just have to "trust" that an implementing class does the project scoping in the requests. In other words, the interface doesn't really provide any guarantee. What do you think?

All that a ProjectClient does is add the project ID in the thread context

I'd think this is an implementation detail and not what the interface should care. To me, ProjectClient is a business concept which really just means this client is supposed to work with a single project (i.e. it's scope, denoted by its interface face getProjectId()), while FilterClient is an implementation choice. Like you said, it does not even have to be implemented with FilterClient.

the interface doesn't really provide any guarantee

I am not sure whether this really is a problem. We can argue that Client#filterWithHeader method does not provide any guarantee either. The contract can be in the javadoc (JDK's own interfaces do that). If we do feel some more hints are needed, we can augument the interface to be something like the followings:

interface ProjectClient extends Client { ProjectId getProjectId(); void executeOnProject(); }

Ok, you convinced me. Thanks for explaining. I converted ProjectClient to an interface in 63e371b.

server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java

ywangd · 2025-07-31T07:21:14Z

server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java

+            // We construct a dedicated project client for the default project if we're in single project mode. This dedicated project
+            // client is an optimization in that it does not use the project resolver and instead executes the request directly.
+            if (defaultProjectClient == null) {
+                defaultProjectClient = new ProjectClient(this, projectId) {


I think we can also pro-actively create it at instantiation time similar to AdminClient. In a non-MP setup, it is guaranteed that some code will need it.

… constructors

ywangd

LGTM

One comment on whether we can have the defaultProjectClient filed to be final by creating it (default or null) in the constructor.

server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java

ywangd

LGTM

Create dedicated ProjectClient class

0ee5d54

A dedicated `ProjectClient` class will make it easier to guarantee a project scope on a type/compilation basis.

nielsbauman added the >non-issue label Jul 31, 2025

elasticsearchmachine added v9.2.0 needs:triage Requires assignment of a team area label labels Jul 31, 2025

nielsbauman commented Jul 31, 2025

View reviewed changes

nielsbauman added :Core/Infra/Core Core issues without another label and removed needs:triage Requires assignment of a team area label labels Jul 31, 2025

nielsbauman requested a review from ywangd July 31, 2025 06:53

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 31, 2025

ywangd reviewed Jul 31, 2025

View reviewed changes

nielsbauman requested a review from tvernum July 31, 2025 07:30

nielsbauman added 2 commits August 1, 2025 11:25

Merge branch 'main' into project-client

bcf22f9

Make ProjectClient an interface

63e371b

nielsbauman requested a review from ywangd August 1, 2025 09:41

Construct default project client lazily to avoid cyclic dependency in…

8f300bf

… constructors

ywangd approved these changes Aug 4, 2025

View reviewed changes

ywangd reviewed Aug 4, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java Show resolved Hide resolved

nielsbauman added 2 commits August 4, 2025 09:47

PR feedback

03e4750

Merge branch 'main' into project-client

67eb8ac

nielsbauman requested a review from ywangd August 4, 2025 07:47

nielsbauman added 2 commits August 6, 2025 11:03

Merge branch 'main' into project-client

93c91bb

Remove project resolver check

f371275

ywangd approved these changes Aug 6, 2025

View reviewed changes

Merge branch 'main' into project-client

12a7ad2

nielsbauman changed the title ~~Create dedicated ProjectClient class~~ Introduce a dedicated ProjectClient interface Aug 7, 2025

nielsbauman enabled auto-merge (squash) August 7, 2025 09:56

nielsbauman disabled auto-merge August 7, 2025 09:56

nielsbauman enabled auto-merge (squash) August 7, 2025 09:56

nielsbauman merged commit f7b7faf into elastic:main Aug 7, 2025
33 checks passed

nielsbauman deleted the project-client branch August 7, 2025 11:01

Introduce a dedicated ProjectClient interface #132237

Introduce a dedicated ProjectClient interface #132237

Uh oh!

Conversation

nielsbauman commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

elasticsearchmachine commented Jul 31, 2025

Uh oh!

ywangd left a comment

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!

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Introduce a dedicated `ProjectClient` interface #132237

Introduce a dedicated `ProjectClient` interface #132237

nielsbauman commented Jul 31, 2025 •

edited

Loading