Rename Thrift fields to avoid reserved words (ie end) while preserving JSON compatibility
#269
Conversation
rtyley
changed the title
end field that uses reserved Thrift wordend
rtyley
changed the title
endend) while preserving JSON compatibility
rtyley
changed the title
end) while preserving JSON compatibilityend) while preserving JSON compatibility
To keep using the old names in the json produced and consumed by these encoders and decoders, it should be possible to use circe-generic-extras to define configured encoders and decoders that do the field renaming. However, this currently fails because deriveConfiguredEncoder and deriveConfiguredDecoder can’t tell that it’s possible to use fezziwig to get the necessary shapeless representations they can normally get already for case classes and traits. It might be possible to update fezziwig to support using it with circe-generic-extras (by outputting whatever deriveConfiguredEncoder needs that deriveEncoder doesn’t), but it’s probably easier to avoid using circe-generic-extras altogether: I’ll investigate that next. (Keeping this commit around for history.)
Since the changes required are pretty straightforward, it’s easier to use .prepare and .mapJson rather than get circe-generic-extras working with fezziwig. However, this does mean we need to check this code quite carefully: working directly with Json means we lose a lot of static typing guarantees, which we will want to get back with tests. (For example, as it is this doesn’t handle the case where only one of the fields is present correctly!)
|
I expect the changes I have pushed should get the tests passing on this branch. However, this does not mean they work fully yet! We need to add more tests to confirm, for example one which attempts to encode/decode json with only one of the startDate and endDate fields (which I expect to fail at the moment). It might also be nice to have some tests verifying that code using the old thrift definitions pre-renaming and code using the new thrift definitions post-renaming will be able to communicate back and forth using either json (from these circe encoders and decoders) or thrift binary encoding. |
These tests exercise the case I mentioned in the last commit, by omitting the start date from the latter test’s json. Fixing the bug should fix this test, I’ll do that next.
This commit fixes the renaming of fields by still working if one of them is missing. (Replacing the single for-expression with two composed .maps.)
|
I wanted to confirm that we haven’t used any other reserved words, so I did some digging to find the list. However, it turns out thrift actually only defines reserved words per-language. So it is scrooge’s mistake to be forcing us not to use end, when end does not appear in the list of java reserved words. For reference, the latter are currently:
I will search for any uses of those in our thrift definitions. |
I think this search means we’re safe. |
rtyley
changed the title
end) while preserving JSON compatibilityend) while preserving JSON compatibility
|
We should check if this field name is also used in thrift in flexible-model, and whether we need to make a similar change there. |
It looks like the same field name exists in flexible-model in ElementFields and MembershipElementFields. What I’m not sure of is how exactly these thrift definitions are used: are they used for any serialisation/deserialisation that uses the field names? |
|
I have raised https://github.com/guardian/flexible-model/pull/85 to rename the fields in flexible-model. There aren’t any jsony uses of the field names in that model that I can see, but I think we’ll probably need to check the apps that use this library, especially any with dependencies on fezziwig. Doing some searching, I think the apps which depend on flexible-model are mainly porter, composer itself, and workflow. |
When completed, this will unblock us from using the most recent versions of Thrift & Scrooge, which started rejecting Thrift definitions that uses the reserved word
endsome time ago:This has had a heavy knock-on effect at the Guardian as we've had Thrift fields named
endin this repo since its initial commit in 2016:content-api-models/src/main/thrift/content/v1.thrift
Lines 455 to 456 in 161af11
To avoiding renaming the fields we've had to resort to using older versions of these libraries that don't have these restrictions:
In order to ensure we can use the most recent versions of those libraries, we need to pay the price: we need to rename the fields.
Testing
Happily, there is a great test within this repo that tests for JSON-conformance for one of the affected Thrift types:
content-api-models/json/src/test/scala/com/gu/contentapi/json/CirceRoundTripSpec.scala
Lines 134 to 136 in 98ea09c
This test fails if we just naively rename the
endfield - this will be a starting point for checking that we remain compatible with JSON parsers.See also