Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: typed component client #1685

Merged
merged 28 commits into from
Jun 27, 2023
Merged

feat: typed component client #1685

merged 28 commits into from
Jun 27, 2023

Conversation

aludwiko
Copy link
Contributor

@aludwiko aludwiko commented Jun 16, 2023

References #1335

Ready for review, in the following PRs I'm planning to:

  • support Workflows
  • clean the double URI Uri construction
  • add more specific tests, params with null values, etc.
  • document it
  • compound keys/ids support, TBD

@aludwiko aludwiko linked an issue Jun 16, 2023 that may be closed by this pull request
@github-actions github-actions bot added java-sdk kalix-runtime Runtime and SDKs sub-team labels Jun 16, 2023
@aludwiko aludwiko marked this pull request as ready for review June 20, 2023 09:28
@aludwiko
Copy link
Contributor Author

Back to draft again, one more issue with encoding :/

@aludwiko aludwiko marked this pull request as draft June 20, 2023 10:17
@aludwiko
Copy link
Contributor Author

ready for review again

@aludwiko aludwiko marked this pull request as ready for review June 20, 2023 12:42
* limitations under the License.
*/

package kalix.javasdk.impl.client
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about packaging here and for MethodRefResolver

Copy link
Member

@octonato octonato Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MethodRefResolver should stay in impl as it is not directly exposed to end users, if I'm not mistaken.

On the other hand, ComponentCall to ComponentCall22 must be public and we need to offer guarantees. Also good if we make them final.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComponentCall still need to move to public package. It's user land api.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final added

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a bit problematic, because it's a scala code, or you mean to just change the package name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think I should rewrite it in Java?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, moved to a proper package

@@ -16,12 +16,15 @@

package kalix.spring.impl
Copy link
Contributor Author

@aludwiko aludwiko Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok ready for review, I simplified the old code a bit and add separate methods for the typed client.

…-kalixclient

# Conflicts:
#	sdk/java-sdk-spring/src/it/java/com/example/wiring/workflowentities/TransferWorkflow.java
#	sdk/java-sdk-spring/src/it/java/com/example/wiring/workflowentities/TransferWorkflowWithFraudDetection.java
#	sdk/java-sdk-spring/src/it/java/com/example/wiring/workflowentities/TransferWorkflowWithoutInputs.java
#	sdk/java-sdk-spring/src/it/java/com/example/wiring/workflowentities/WorkflowWithDefaultRecoverStrategy.java
#	sdk/java-sdk-spring/src/it/java/com/example/wiring/workflowentities/WorkflowWithRecoverStrategy.java
#	sdk/java-sdk-spring/src/it/java/com/example/wiring/workflowentities/WorkflowWithTimeout.java
#	sdk/java-sdk-spring/src/main/scala/kalix/javasdk/impl/EntityDescriptorFactory.scala
Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I left a few comments.

I will do a second pass after lunch.

return new ActionCallBuilder(kalixClient);
}

public ValueEntityCallBuilder forValueEntity() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that variant intended for methods annotated with @GenerateId?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

* limitations under the License.
*/

package kalix.javasdk.impl.client
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComponentCall still need to move to public package. It's user land api.

Comment on lines 46 to 48
val entityKeysOnType = idValue(component)
val entityKeyOnMethod = idValue(method)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for our own sanity, we should call it entityIds..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, copy paste from the source before the renaming

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And class should be IdExtractor.scala. We can remove the entity prefix in all places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realise now that they are all under kalix.spring. It should be under kalix.javasdk instead.

@aludwiko aludwiko requested a review from octonato June 26, 2023 06:53
Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with some minor suggestions and change requests. Good for merge after that.

* <pre>{@code
* public Effect<String> createUser(String userId, String email, String name) {
* //validation here
* var defCall = componentClient.forValueEntity(userId).call(UserEntity::createUser).params(email, name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have this example code. 💯

We are using pure java, but that's not a common style. Good to have a clear example.

@octonato octonato merged commit b85f29a into main Jun 27, 2023
65 checks passed
@octonato octonato deleted the 1335-spring-sdk-typed-kalixclient branch June 27, 2023 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spring SDK: Typed KalixClient
2 participants