Vsp over object v3 mc #245

marcin-cebo · 2022-09-06T07:39:08Z

1.No sure if we want to put deprecation messages on ObjectV2 method just now because ObjectV3 REST API doesn't seem to be in final shape.
2.I realised that ObjectV3 Rest API doesn't return updated and eTag fields.

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

kleewho

Some of the comments are applicable to more places

kleewho · 2022-09-07T08:03:54Z

src/main/java/com/pubnub/api/endpoints/objects_vsp/user/CreateUser.java

+
+import java.util.Map;
+
+public abstract class CreateUser extends UserEndpoint<CreateUser, EntityEnvelope<User>, CreateUserResult> implements CustomIncludeAware<CreateUser> {


This could be an interface. It'll save us few lines. This requires few additional changes in generic parameter boundaries from Endpoint to RemoteAction. This how the full interface would look like:

public interface CreateUser extends RemoteAction<CreateUserResult>, CustomIncludeAware<CreateUser> { CreateUser name(String name); CreateUser email(String email); CreateUser profileUrl(String profileUrl); CreateUser externalId(String externalId); CreateUser custom(Map<String, Object> custom); CreateUser status(String status); CreateUser type(String type); CreateUser userId(UserId userId); static CreateUser create( final PubNub pubNub, final TelemetryManager telemetryManager, final RetrofitManager retrofitManager, final TokenManager tokenManager) { final CompositeParameterEnricher compositeParameterEnricher = CompositeParameterEnricher.createDefault(); return new CreateUserCommand(pubNub, telemetryManager, retrofitManager, tokenManager, compositeParameterEnricher); } }

I will contact you to talk about this change.

Sure. Let's talk

I tried to introduce interface but is seems more easier approach to reduce code we can get rid of CreateUserCommand class and move all login from that class to CreateUser that will be changed from abstract to non abstract class. I also removed UserEndoint class.

kleewho · 2022-09-07T08:05:45Z

src/main/java/com/pubnub/api/endpoints/objects_vsp/user/CreateUserCommand.java

+    private String externalId;
+    private Map<String, Object> custom;
+    private String status;
+    private String type;


All those fields could have generated fluent setters with lombok, just like we have for most of other implementations. It'll save us few lines. It'll look like this:

@Accessors(chain = true, fluent = true) public final class CreateUserCommand extends CreateUser implements HavingCustomInclude<CreateUser> { @Setter private String name;

I can change it without big enthusiasm. With this construction code is more difficult to read for people that don't know lombok. But this is good chance for me to learn it :)
FYI
In SetChannelMetadataCommand and SetUUIDMetadataCommand this lombok is not used.

I'm not a fan of lombok either, but if we have it already and it's used in quite a few places we can as well just make use of it

In SetChannelMetadataCommand and SetUUIDMetadataCommand this lombok is not used.

That might have been an oversight. AFAIR it was a big big PR as well, so I might have missed something

kleewho · 2022-09-07T08:06:59Z

src/main/java/com/pubnub/api/endpoints/objects_vsp/user/CreateUserCommand.java

+import java.util.HashMap;
+import java.util.Map;
+
+public final class CreateUserCommand extends CreateUser implements HavingCustomInclude<CreateUser> {


This is an implementation so I'd make it package private. There should be no need for customer to use these

I like this idea.

kleewho · 2022-09-07T08:27:57Z

src/main/java/com/pubnub/api/PubNub.java

    public SetChannelMetadata.Builder setChannelMetadata() {
        return SetChannelMetadata.builder(this, this.telemetryManager, this.retrofitManager, this.tokenManager);
    }

+    public CreateSpace createSpace() {


I would likely see here all required parameters instead of everything being optional. That would mean that all space related methods should have spaceId at least. This could bring additional changes in other places: making it required in the *SpaceCommand and also most probably the SpaceEndpoint will no longer be necessary

I like this idea.

I knew that you'll like it :)

kleewho · 2022-09-07T08:38:57Z

src/main/java/com/pubnub/api/endpoints/objects_vsp/UserEndpoint.java

+    protected UserId effectiveUserId() {
+        try {
+            return (userId != null) ? userId : getPubnub().getConfiguration().getUserId();
+        } catch (PubNubException e) {


Seeing this catch I think we made a mistake by keeping uuid in configuration as a base field instead of UserId. We wouldn't have to have this here for simple getUserId

kleewho · 2022-09-07T08:42:54Z

src/main/java/com/pubnub/api/models/consumer/objects_vsp/space/FetchSpaceResult.java

+
+@NoArgsConstructor
+public class FetchSpaceResult extends SpaceResult {
+    public FetchSpaceResult(EntityEnvelope<Space> entityEnvelope) {


I guess you missed final

Yes, thanks.

kleewho · 2022-09-07T09:01:52Z

src/main/java/com/pubnub/api/services/vsp/SpaceService.java

+public interface SpaceService {
+
+    @POST("/v3/objects/{subKey}/spaces/{spaceId}")
+    @Headers("Content-Type: application/json; charset=UTF-8")


Shouldn't we include it everywhere where we have Body or not include it at all?

I will delete it.
I found @headers in ChannelMetadataService and UUIDMetadataService so I will delete them from there as well.

kleewho · 2022-09-07T09:02:41Z

src/main/java/com/pubnub/api/models/server/objects_vsp/user/UserPayload.java

+@Getter
+@ToString
+@AllArgsConstructor
+public abstract class UserPayload {


I'd use just @Data instead of these annotations

@Data introduces @Setter. I am wondering if we want to have setters for those fields?

it doesn't if you have everything final

kleewho · 2022-09-07T09:03:47Z

src/main/java/com/pubnub/api/models/server/objects_vsp/space/SpacePayload.java

+@Getter
+@ToString
+@AllArgsConstructor
+public abstract class SpacePayload {


@Data should be all right here instead of currently present

Makes sense. I will change it.

kleewho · 2022-09-07T13:01:37Z

src/main/java/com/pubnub/api/endpoints/objects_vsp/space/CreateSpace.java

+
+import java.util.Map;
+
+public abstract class CreateSpace extends SpaceEndpoint<CreateSpace, EntityEnvelope<Space>, CreateSpaceResult> implements CustomIncludeAware<CreateSpace> {


Why the return value is CreateSpaceResult and not just Space? In Kotlin we return just Space. Could you please align the return values for all new methods to what we have in Kotlin already?

I forgot that in Kotlin we get rid of status in response. I will change CreateSpaceResult to Space. Status might be inferred by Space object being present or exception being thrown
e.g. when creating space with id that already exists following exception is thrown:
{"error":{"message":"Requested resource already exists.","source":"metadata"},"status":409}

kleewho

The ObjectsV2 implementation is one of the most complicated things in the JavaSDK and I think we went over the top with this. I don't think it needs to be as complicated. I'm not sure what you think about it, but feel free to drop all the complicated bits of that implementation if you share this impression

kleewho · 2022-09-09T09:13:16Z

src/main/java/com/pubnub/api/models/consumer/objects_vsp/space/Space.java

+@EqualsAndHashCode(callSuper = true)
+@ToString(callSuper = true)
+@NoArgsConstructor
+public class Space extends PNObject {


Shouldn't we have a SpaceId as a type for id field for this class?

Yes. Good catch. I will create class similar to PNObject but with no id and add:
private SpaceId id;
to Space.java.
I will also add custom interceptor to map id as String in REST response to SpaceId .

public class SpaceIdDeserializer implements JsonDeserializer {
@SneakyThrows
@OverRide
public SpaceId deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException {
String spaceIdValue = json.getAsString();
return new SpaceId(spaceIdValue);
}
}

kleewho · 2022-09-09T09:17:53Z

src/main/java/com/pubnub/api/models/consumer/objects_vsp/user/User.java

+@EqualsAndHashCode(callSuper = true)
+@ToString(callSuper = true)
+@NoArgsConstructor
+public class User extends PNObject {


Same story as with Space. I think its id should have type UserId

I will add it. Thanks.

kleewho · 2022-09-09T09:27:57Z

src/main/java/com/pubnub/api/models/consumer/objects_vsp/space/RemoveSpaceResult.java

+@Setter(AccessLevel.PACKAGE)
+@NoArgsConstructor
+@ToString(callSuper = true)
+public class RemoveSpaceResult extends EntityEnvelope<JsonElement> {


Simpler alternative

@Data public class RemoveSpaceResult { private final int status; }

Ok. I will use magic of lombok.

kleewho · 2022-09-09T09:36:53Z

src/main/java/com/pubnub/api/models/consumer/objects_vsp/space/Space.java

+    }
+
+    @Override
+    public PNObject setCustom(Object custom) {


I think custom should not be an Object but rather should have the same type as we expect when we createSpace, so Map<String, Object>.

What would you say about this simpler version of Space?

@Data public class Space { @NonNull private final SpaceId id; private final String name; private final String description; private final String status; private final String type; private final Map<String, Object> custom; }

probably we'd need custom deserializer for SpaceId

I think this is good improvement. I guess we need to add eTag and updated field so:

@EqualsAndHashCode(onlyExplicitlyIncluded = true)
@DaTa
public class Space extends ObjectVsp {

@EqualsAndHashCode.Include @JsonAdapter(SpaceIdDeserializer.class) @Setter private SpaceId id; private String name; private String description; private String status; private String type; @Override public Space setCustom(Map<String, Object> custom) { super.setCustom(custom); return this; }

}

@DaTa
@accessors(chain = true)
public class ObjectVsp {

@JsonAdapter(CustomPayloadJsonInterceptor.class) @Setter protected Map<String, Object> custom; protected String updated; protected String eTag;

}

kleewho · 2022-09-09T09:42:02Z

src/main/java/com/pubnub/api/endpoints/objects_vsp/space/CreateSpaceCommand.java

+        if (input.body() != null) {
+            return input.body().getData();
+        } else {
+            return new Space();


I think it should rather be an error. We are expecting body in case of successful response

Not sure what you suggest? Something like this ?
if (input.body() != null) {
return input.body().getData();
} else {
throw PubNubException.builder().pubnubError(PubNubErrorBuilder.PNERROBJ_CHANNEL_MISSING).build();
}
In case of error program will not hit this method. So return new Space() should never happen.

I was looking at PNSetChannelMetadataResult, GetChannelMetadataCommand and there empty object creation but maybe it's more precise to throw exception.

Removed *Command classes Removed UserEndpoint classes Made CreateSpace, FetchSpace..., CreateUser, FetchUser... non abstract

This reverts commit a7e1969.

Introduced interfaces for all User/Space operations e.g. CreateUser, CreateSpace

marcin-cebo added 13 commits August 26, 2022 16:02

Added createUser operation to replace setUUIDMetadata

01c6cc6

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added removeUser operation to replace removeUUIDMetadata

97d8187

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added fetchUser operation to replace getUUIDMetadata

54ad64d

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added updateUser operation to replace setUUIDMetadata

8bbdb29

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added upsertUser operation to replace setUUIDMetadata

eaddbf3

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added upsertUser operation to replace setUUIDMetadata

80173e7

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added createSpace operation to replace setChannelMetadata

447a47c

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added removeSpace operation to replace removeChannelMetadata

606f364

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added removeSpace operation to replace removeChannelMetadata

e0baae7

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added fetchSpace operation to replace getChannelMetadata

1913dd4

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added updateSpace operation to replace setChannelMetadata

9420144

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added upsertSpace operation to replace setChannelMetadata

048ba17

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

Added upsertSpace operation to replace setChannelMetadata

88f9b92

Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.

marcin-cebo requested review from kleewho, Chesteer89 and budgetpreneur as code owners September 6, 2022 07:39

kleewho requested changes Sep 7, 2022

View reviewed changes

kleewho reviewed Sep 7, 2022

View reviewed changes

marcin-cebo added 3 commits September 8, 2022 17:35

ObjectV3 user/space changes after code review.

7ac0e9f

ObjectV3 user/space changes after code review.

4285deb

ObjectV3 user/space changes after code review.

7d9852e

kleewho requested changes Sep 9, 2022

View reviewed changes

marcin-cebo added 5 commits September 9, 2022 12:44

ObjectV3 user/space changes after code review.

ba24d8d

ObjectV3 user/space changes after code review.

1fede60

ObjectV3 user/space changes after code review.

a7e1969

Removed *Command classes Removed UserEndpoint classes Made CreateSpace, FetchSpace..., CreateUser, FetchUser... non abstract

Revert "ObjectV3 user/space changes after code review."

30339ac

This reverts commit a7e1969.

ObjectV3 user/space changes after code review.

8c7a637

Introduced interfaces for all User/Space operations e.g. CreateUser, CreateSpace

kleewho approved these changes Sep 14, 2022

View reviewed changes


		import java.util.Map;

		public abstract class CreateUser extends UserEndpoint<CreateUser, EntityEnvelope<User>, CreateUserResult> implements CustomIncludeAware<CreateUser> {


		import java.util.Map;

		public abstract class CreateSpace extends SpaceEndpoint<CreateSpace, EntityEnvelope<Space>, CreateSpaceResult> implements CustomIncludeAware<CreateSpace> {

Vsp over object v3 mc #245

Are you sure you want to change the base?

Vsp over object v3 mc #245

Conversation

marcin-cebo commented Sep 6, 2022

kleewho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-cebo Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleewho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleewho Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-cebo Sep 8, 2022 •

edited

Loading

kleewho Sep 9, 2022 •

edited

Loading