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(KNO-5026): Add tenant inline identification #32

Conversation

francoborr
Copy link
Contributor

Updates the following java classes: WorkflowTriggerRequest, UpdateSchedulesRequest and CreateSchedulesRequest

Until now the tenant property in these classes was a simple string, but from now on the property can have one of the following values:

  • A String ID
  • A TenantRecipientIdentifier ({"id": "tenant-id"})
  • A map full of custom properties.

@@ -56,6 +55,26 @@ public UpdateSchedulesRequestBuilder addActor(String actor) {
return this;
}

public UpdateSchedulesRequestBuilder addActor(Map<String, Object> actor) {
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 think this is something we missed, as actors can also be passed as a map full of custom properties.

@francoborr francoborr changed the title feat(KNO-5026): Add tenant inline identification support feat(KNO-5026): Add tenant inline identification Jan 18, 2024
@@ -28,7 +27,7 @@ public class CreateSchedulesRequest {
List<Object> recipients;
List<ScheduleRepeat> repeats;
Object actor;
String tenant;
Object tenant;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this type incorporate a string or a Tenant object then? Object is like any in typescript, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So for both actor and tenant we support the following values:

  • A simple string
  • A more complex object with properties

So answering to your question: Yes from my understanding Object works as a generic type (and can be a string as well), so it covers the possible values we may have IMO.

Comment on lines 67 to 68
.tenant(TenantRecipientIdentifier.builder()
.id("tenant-id")
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing this in use, I don't think we need this builder as all we're ever doing is building a string?

Copy link
Contributor Author

@francoborr francoborr Jan 24, 2024

Choose a reason for hiding this comment

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

So there are three posible values for a tenant:

  • A raw string referencing the tenant ID
  • An object containing only an ID property(for this we use the TenantRecipientIdentifier builder we are talking about)
  • An object with more properties

So from my understanding we need this builder for the second case, as I want to be clear a tenant can be an object with a simple ID field without more properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

your example of (2) is wholly covered under (3). We only had this builder for objects because you can explicitly build an object reference that needs an id and a collection, so the builder was a convenience method to build that elegantly, without needing to pass a map.

For tenants, the interface should just be either a string, or a map.

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 got it, I'll remove the builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@francoborr francoborr requested a review from cjbell January 24, 2024 14:08
@francoborr francoborr merged commit 1a59dc0 into main Jan 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants