[client] Write clients to the same package

[SentryMan]

Is there a need for them to be in a separate package?

also make them final.

[rbygrave]

Is there a need for them to be in a separate package?

For @Client.Import we are generating them for an interface that could be in another java module - so we can't use that same package.

[SentryMan]

Handled

[rbygrave]

Why do you want them in the same package? Why do you think that is better than having everything in .httpclient ?

[ferrazoli]

Having the HttpClient in another package forces the developer to keep all client-related classes public, otherwise they won't be visible by the HttpClient. This causes a lot of namespace pollution.

[rbygrave]

developer to keep all client-related classes public

It's only the client interface - that's it. That client interface pretty much has to be public to be useful.

[rbygrave]

To me this has negative impact on java module exports. That is, currently we can choose to export a package that only contains the interface with the option of exporting the package with the generated code/implementation or not. My expectation is that we often do not want the generated code in the exported package.

[SentryMan]

@ferrazoli do you have anything else to add?

[ferrazoli]

It's only the client interface - that's it. That client interface pretty much has to be public to be useful.

I don't understand why you'd think that. Consider the following project structure:

org.example.batman.BatmanEventProducer
org.example.avaje.AvajeEventProducer
org.example.client.EventConsumer
org.example.client.ExternalService
org.example.client.Foo
org.example.client.FooResponse
org.example.client.Bar
org.example.client.BarResponse

Consider that ExternalService can be something like:

@Client
interface ExternalService {

  @Post("/foo")
  FooResponse foo(Foo foo);

  @Post("/bar")
  BarResponse bar(Bar bar);

}

In this example, EventConsumer takes events from both BatmanEventProducer and AvajeEventProducer, and interacts with some external service defined in ExternalService. It could then log the responses in a database.

Here, not only there is no need for the client to be public, but there's also no reason why Foo, Bar, FooResponse, BarResponse, or even ExternalService should be accessible by the avaje, batman or any other package in the project. To me, they should be self contained in client unless I have a reason to expose them outside of that package. However, this code will not compile if any of these resources are not public, because they will not be accessible in org.example.client.httpclient, which is where the generated ExternalServiceHttpClient will be located.

In this example, there are few resources involved, but in my real life example that led to this request, I have over 20 different resources that are related to an external API for product analytics, some of which have very generic names such as Track, Event, Group, Identity, etc. It's really annoying to have these resources polluting the global namespace.

In an ideal world, Java has package hierarchy structures where I can make these resources available to sub-packages, but not to others, but we're not living in that world. 😄

I can't comment on what was said about module exports, but I just think that the generated code should respect my package visibility choices, which is something that avaje-inject does, but not avaje-http-client.

Does this make sense or am I structuring my project in a silly way? I'm open to the possibility that there's a better way to do things which would eliminate my issue in the first place. Please advise.

[rbygrave]

generated code should respect my package visibility choices

It could and it probably should. It's just not what this PR does currently.

That is, I'm thinking this PR could change to satisfy both requirements. If the client interface is not public use the same package [to satisfy use case (B)], and when it is public use the .httpclient package [to satisfy use case (A)].

what was said about module exports

Effectively there are 2 use cases to consider.
(A) Public API - An http client for that API that we might publish to a maven repo and reuse
(B) Internal Use Only - An http client that is only for internal use - not for reuse, not published to maven repo/nexus etc.

For the (A) case wrt java modules we really desire to export only the interface and we want to keep the actual implementation internal / not accessible such that others don't accidentally start depending on the implementation.

It's only the client interface - that's it

Yeah I got that wrong in that I forgot that also applied to Foo, FooResponse, Bar, BarResponse etc. I forgot about use case (B).

in my real life example ..., I have over 20 different resources ... very generic names ... annoying to have these resources polluting the global namespace.

Thanks for that - I get it and this is exactly the sort of information we are after to understand the motivation any given change. For this case, this PR was created without that motivation explicitly stated so we've bounced around a bit here. Ideally for me, the motivation for ANY change is stated up front because we can equally "test the motivation" and look at the motivation for issues / flaws / side effects and that give us some confidence that we are changing things for the right reasons etc.

edit:

something that avaje-inject does, but not avaje-http-client

This is the case because from a multi-module perspective DI components are basically mostly "internal" [not exported to other modules] where as for avaje-http-client we have use case (A) where we desire to only export the interface and keep the implementation internal.

[SentryMan]

It's just not what this PR does currently.

It does now

[SentryMan]

@rbygrave can we get another RC for this?

[ferrazoli]

Thanks @rbygrave and @SentryMan ! ❤️