-
Notifications
You must be signed in to change notification settings - Fork 40
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
Support outbound message extras #581
Conversation
…a JsonObject. Some refactoring here to improve our use of gson also.
…reated from JSON does not return a delta extras instance.
… bidirectional coding of 'raw' arbitrary JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise that this is still draft. Do you think we should also now support PushExtras
?
private final DeltaExtras delta; | ||
|
||
/** | ||
* Only intended for use with MessageExtras instances created to be sent to Ably's servers (outbound). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the comment. Supporting raw
is for content that isn't understood by this library, and I think that content can go in both directions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I think it's evolved since our discussion later yesterday. I shall refine this commentary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paddybyers In terms of supporting I know that the customer who is waiting on us fixing this omission is already generating the necessary gson object in their own code - according to this mould/example that they gave us: JsonUtils.`object`().add(
"push", JsonUtils.`object`().add(
"fcm", JsonUtils.`object`()
// more config here
)
).toJson() (via internal Slack message) As such, they can keep that same code and just utilise the new constructor I've added to pass 'raw' gson. I'm happy to change that terminology if it's considered incorrect or somehow otherwise misleading. 😁 |
…raw is available, they only use that value. Also adds a toString, which assists with test debugging apart from anything else.
It fails for the text protocol at the moment due to an issue with inbound JSON decoding.
…only inspect raw if it's available.
…e old version we had in place (November 2015).
…n message extras), fixing the implementation to support this.
Back to the drawing board again, I'm afraid. Some of this suite failing:
|
…was causing REST crypto tests for text protocol to fail.
|
||
/** | ||
* @since 1.2.0 | ||
*/ | ||
public MessageExtras(final DeltaExtras delta) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not clear on how this API is going to grow to accommodate more known extras
members. We're going to have push
, metadata
, etc. How would you construct a MessageExtras
if you have multiple of those values to supply?
The kind of thing you're seeing in other languages now is where the type for each of the known members implements a specific interface, eg
interface MessageExtra {}
class DeltaExtras implements MessageExtra {
...
}
class PushExtras implements MessageExtra {
...
}
public MessageExtras(JSONObject raw, MessageExtra ...members) { .... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Depending on pressure we're under to release 1.2.1 it would definitely be worth having a call to discuss this as you suggested elsewhere.
The main thing I don't want to do is make any breaking changes between 1.2.0 and 1.2.1, but what you propose merely widens the type acceptance of this method so I think that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paddybyers I'm currently disinclined to do anything about this in order to get this fix out... we can think how to architect this properly outside of the scope of this PR, if that's ok with you?
In terms of what I mentioned above... If we added a super interface at some point in the future to DeltaExtras
then this method could be safely refactored to accept any object that implements that interface without that being a breaking API change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public MessageExtras(final DeltaExtras delta)
who will use this API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason or another it looks like it's only used by RealtimeDeltaDecoderTest at the moment. I'll have a look as to whether that's needed. Perhaps I can replace it with a mock.
Meanwhile, of course, do we simply ignore the fact that we exposed this for 1.2.0? Maybe the best option is to simply get rid of it from the public API. What do you think, @paddybyers?
(equally I guess you could follow that argument through and just revert this portion of the change to the type of message extras
wholesale!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed that API: d6ff228
/* package private */ JsonElement toJsonElement() { | ||
return Serialisation.gson.toJsonTree(this); | ||
/* package private */ JsonObject getRaw() { | ||
return raw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need to be public
? If someone validly sends me a message containing a member in extras
that this library doesn't know about, then I'm going to have to process the raw JSON.
If we are going to make it public, then perhaps it needs to be asJSON()
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to make that public, yes. I was just trying to minimise exposure but I can see that's a clearly valid use case. I agree that, in that case, the name would need to change too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now made that method public: e41c806
…lic. Also: - some renaming for clarity - add code to the DeltaExtras constructor to ensure that a JsonObject is always available, making the asJsonObject method easier to reason about
…ase where a key is populated but with JSON null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@QuintinWillison I like the idea of keeping an internal I think we can improve the way we hydrate the DeltaExtras object. MessageExtras can become simpler as well. In the constructor you will just take the json and parse it to hydrate the deltas object. No need to have different parsing for json and message pack. You can use the same logic you have when parsing the json. Something like this:
This way we can easily add more logic there to extract other read-only or read-wrtie objects. Objects that will support writing to the json will have to have an instance of the internal json object passed to them. The next thing to note is that we should always return a copy of the jsonObject because it is mutable and we don't want people to be changing it under our feet. If somebody wants to update the MessageExtras of a Message they should use the constructor. |
Thanks @marto83 ... if this is all implementation detail then I'm inclined to leave things as they are so we can get this release out. I'm happy to work on internal refactoring later and am also happy for you to take an alternative approach when you make the same change to the .NET client library. |
I agree, they are all internal details. I'm happy with the external interface. |
Fixes #580. Conforms, IMO, with TM2i (with a little bit of idiomatic artistic license).
Tasks outstanding:
MessageExtras
unit tests (both constructors and message pack)