search

nats-io / nats.java

Java client for NATS
Apache License 2.0
563 stars 153 forks source link

Serializable Wrappers #1156

Closed scottf closed 2 months ago

scottf commented 3 months ago

It's impractical to make configuration object serializable because it would make any changes like an addition of a field NOT backward compatible because the serialVersionUID would change, versioning the entire class, requiring a semver major version.

So instead,

somratdnutanix commented 3 months ago

Thanks @scottf . The changes look solid. Please ensure the tests cover all possible configurations, including edge cases, for both serialization and deserialization. We can include tests for error scenarios such as handling invalid JSON input. Once its merged, I can refactor the connector code to use this instead of what we have used now.

scottf commented 3 months ago

Please ensure the tests cover all possible configurations, including edge cases, for both serialization and deserialization

What edge cases? If the json is invalid, it will throw a parse error. Also since I run the json through the builder invalid data should be caught there but...I don't think it's necessary to handle edge cases. The expectation is that the json comes from the objects' toJson method so should never be invalid. The unit tests all test round trip of the serialization. Is there something I am missing?

somratdnutanix commented 2 months ago

Okay, then it should be pretty much covered @scottf. Looks good. 👍

somratdnutanix commented 2 months ago

It's impractical to make configuration object serializable because it would make any changes like an addition of a field NOT backward compatible because the serialVersionUID would change, versioning the entire class, requiring a semver major version.

So instead,

  • relevant classes are ensured to be JsonSerializable if not already
  • builders are modified to accept json.
  • wrapper classes have been added that use the json to serialize and deserialize

thanks @scottf for this. we won't have to use proxy pattern for each field now. will refactor our connector code and be back with you.

somratdnutanix commented 2 months ago

got these warning when I tried to build Maven locally, mostly due to missing Javadoc comments, @scottf .

> Task :javadoc ConsumerConfiguration.java:697: warning: no @param for json public Builder json(String json) throws JsonParseException { ^ ConsumerConfiguration.java:697: warning: no @return public Builder json(String json) throws JsonParseException { ^ ConsumerConfiguration.java:697: warning: no @throws for io.nats.client.support.JsonParseException public Builder json(String json) throws JsonParseException { ^ ConsumerConfiguration.java:704: warning: no @param for v public Builder jsonValue(JsonValue v) { ^ ConsumerConfiguration.java:704: warning: no @return public Builder jsonValue(JsonValue v) { ^ ConsumerConfiguration.java:650: warning: no @param for cc public Builder(ConsumerConfiguration cc) { ^ BaseConsumeOptions.java:124: warning: no @param for v public B jsonValue(JsonValue v) { ^ BaseConsumeOptions.java:124: warning: no @return public B jsonValue(JsonValue v) { ^ 8 warnings

scottf commented 2 months ago

got these warning when I tried to build Maven locally, mostly due to missing Javadoc comments, @scottf Warnings right? I'll fix the java doc, thanks