PascalCase enum members for endpoint responses

[letmaik]

The State enum members are lowerCamelCase. Other enums are PascalCase (e.g. TxStatus). The JSON serialization converts some of the enums to UPPERCASE via custom functions.

The Microsoft REST API guidelines say that enum members should be lowerCamelCase. I suggest either standardizing on lowerCameCase in the code (even though it looks a bit funny) and removing any custom conversion functions, or using PascalCase in the code and adding/changing conversion functions everywhere to make them lowerCamelCase in endpoint responses.

This also has an effect on enum members used as part of a request, e.g. status query param in /network/nodes, so it has to work both directions.

[eddyashton]

I'm not sure what you mean by custom conversion functions - to convert the C++ enum values to JSON we need a NLOHMANN_JSON_SERIALIZE_ENUM declaration, which we wrap as DECLARE_JSON_ENUM, to indicate how each value should be converted. There's no default stringification, so these are necessary. We can standardise the case for them all (and the case of the C++ enums, which we don't have a strict policy on), but we still need to declare the stringification for each value somewhere.

[letmaik]

Ah right, thanks for clarifying, I missed that.

[letmaik]

This is what's exposed as part of the REST API at the moment: enum name -> JSON values

CodeStatus: ALLOWED_TO_JOIN
TxStatus: UNKNOWN, ...
ProposalState: OPEN, ...
ServiceStatus: OPENING, ...
NodeStatus: PENDING, ...
State: uninitialized, ...

For some reason TxStatus has a separate function tx_status_to_str to do the conversion whereas all other enums use a string with DECLARE_JSON_ENUM, probably some left-over code.

I think all uppercase is a bit unusual, but definitely casing should be consistent.

It would also be fine to use PascalCase, since lowerCamelCase is only a recommendation. The nice thing about lowerCamelCase is that single-word values are all lowercase, which fits with the rest of the JSON.

[eddyashton]

The advantage of using a separate function rather than using a string literal, is that you can then reuse the same stringification elsewhere (eg - in error messages). We were doing this in more locations for TxStatus, though its now only used in a single error message. I'm sure we duplicate this stringification for several of these types (either writing the string literal directly in an error/logging message, or formatters like formatter<ccf::NodeStatus> in nodes.h).

I thought constants were usually SHOUTY_CASE in JS, is this not also the case in JSON?

[letmaik]

It's not that common in REST APIs, note that this is separate to how things are named in code. We should roughly align to what Azure is using, even though that's not completely consistent:

https://github.com/Azure/azure-rest-api-specs/search?q=enum

[letmaik]

Had another look at the Azure link, the vast majority of the APIs uses PascalCase. Everyone ok with that?

[wintersteiger]

The only thing about that it that I don't like is that acronyms are capitalized weirdly, e.g. RsaOaep. I think we should allow ourselves to call that RSAOAEP.

[eddyashton]

I'm happy with PascalCase for JSON enum members.

I don't mind the weird acronym capitalization. I think RsaOaep is often more readable, because if I don't know what an RSASSLDESSHAECD is I probably can't Microsoft Bing it, where I at least stand a chance of decomposing RsaSslDesShaEcd.

[wintersteiger]

Yeah, that's a fair point @eddyashton. We'd have to use RSA_OAEP to get that bit of information back.