Skip to content

Introduces reworked Payload API and new Metadata API #117

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 4, 2020
Merged

Introduces reworked Payload API and new Metadata API #117

merged 1 commit into from
Nov 4, 2020

Conversation

whyoleg
Copy link
Member

@whyoleg whyoleg commented Nov 2, 2020

More attention on mime-types, payload and metadata packages and tests for them

@whyoleg
Reworked Payload API
6840aaf 
Introduce Metadata API
Composite Metadata support
Extensions Metadata support (Routing, Tracing, Auth, PerStream)
@whyoleg whyoleg added this to the 0.11.0 milestone Nov 2, 2020
@whyoleg whyoleg requested a review from yschimke November 2, 2020 20:19
@whyoleg whyoleg self-assigned this Nov 2, 2020
@whyoleg whyoleg requested a review from OlegDokuka November 2, 2020 20:19
yschimke
yschimke approved these changes Nov 2, 2020
View reviewed changes
Copy link
Member

@yschimke yschimke left a comment

Choose a reason for hiding this comment

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

I'd like to try with a downstream project and give feedback then

examples/multiplatform-chat/src/clientMain/kotlin/Api.kt
@@ -51,6 +51,6 @@ suspend fun connectToApiUsingTCP(name: String): Api {

private fun connector(name: String): RSocketConnector = RSocketConnector {
connectionConfig {
setupPayload { Payload(name) }
setupPayload { buildPayload { data(name) } }
Copy link
Member

Choose a reason for hiding this comment

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

Why change this line. Seems simpler before

Copy link
Member Author

Choose a reason for hiding this comment

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

Old Payload constructors were removed in favor of buildPayload. They were really like example constructors. F.e. in that particular case it was Payload(data: String, metadata: String? = null) but for sure, such constructor will be needed in very small amount of cases. And f.e. upper I've used general buildPayload to create such narrow case constructor.
So from user perspective, similar functions can be created per use cases they need

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a matter of preference. I'd consider using buildPayload() when I want to incrementally build the payload like buildString(). But if I have the full content already ready, even allowing string -> byte conversions, I'd like Payload(...).

Copy link
Member Author

Choose a reason for hiding this comment

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

main question for me is, Do we really need functions like Payload(String, String?)?

anyway, may be it's better to change name of buildPayload ? to what? Payload {} ? not sure about this

Im not sure that a constructor per combination of data/metadata possible types is good.
So, if we want to support all cases with Payload we need 15 functions for that to combine data=String|BRP|ByteArray and metadata=String|ByteArray|BRP|Metadata|null.
In current solution those constructors can be emulated on user side, if they use it frequently with 3-4 lines of code.

Copy link
Member

Choose a reason for hiding this comment

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

I was only asking for constructor for complete string or byte[] cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will return back string/string and bytes/bytes functions

examples/multiplatform-chat/src/serverJvmMain/kotlin/App.kt
@@ -43,6 +45,8 @@ fun main() {

val rSocketServer = RSocketServer()

fun Payload.route(): String = metadata?.read(RoutingMetadata)?.tags?.first() ?: error("No route provided")
Copy link
Member

@yschimke yschimke Nov 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We should put these as part of the public API for each specific domain via extension methods. Seems here it's a local helper we would duplicate a lot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?
It's really local helper to get one route, for particular example. In real world, getting only first tag will not be enough I assume

Copy link
Member

Choose a reason for hiding this comment

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

I think you gave the best example, for each domain there will be different conventions to use these. Rather that just providing access as a dictionary, it would be nice to have extension methods you can import to make it feel natural to each domain.

playground/src/commonMain/kotlin/Metadata.kt
val tm: ZipkinTracingMetadata = cm.get(ZipkinTracingMetadata)

//or
val rm1: RoutingMetadata = cm[RoutingMetadata]
Copy link
Member

Choose a reason for hiding this comment

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

Think examples like this are better through well known extension methods

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you explain a little?
Or, show better example?

Copy link
Member

Choose a reason for hiding this comment

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

cm.route()

Copy link
Member

Choose a reason for hiding this comment

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

see above.

Copy link
Member Author

Choose a reason for hiding this comment

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

so, you propose to add something like val Payload.routingMetadata: RoutingMetadata or fun Payload.routingMetadata(): RoutingMetadata which can be read once, right?
and same accessors for other out-of-the-box metadatas?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's weird on Payload, but as an extension method you choose to import it.

Also the domain specific helper could hide any logic to check is CompositeMetadata vs single (non-composite) types, or potentially include defaults that make sense for that type.

Copy link
Member Author

Choose a reason for hiding this comment

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

looks ok, will add it

Copy link
Member

Choose a reason for hiding this comment

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

Let's not overthink it. Let's land and iterate for now. I use the routing rules from client side, so let's see that in the context of rsocket-cli and my own project.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, So may be, let's merge it now and you will check it
If so, we can at least release 0.11.0 with those changes already done
and continues with incremental releases per feature

@whyoleg whyoleg merged commit 4935fc7 into master Nov 4, 2020
@whyoleg whyoleg deleted the enhancement/metadata-and-payload-api branch November 4, 2020 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yschimke yschimke yschimke approved these changes

@OlegDokuka OlegDokuka Awaiting requested review from OlegDokuka

Assignees

@whyoleg whyoleg

Labels
breaking-change
Projects
None yet
Milestone
0.11.0
Development

Successfully merging this pull request may close these issues.

Extension metadata Composite Metadata & WellKnownMimeTypes implementation
2 participants
@whyoleg @yschimke