brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Heneveld <alex.henev...@cloudsoftcorp.com>
Subject Re: Brooklyn REST API - omitting fields in JSON objects
Date Wed, 08 Nov 2017 12:24:06 GMT

note previously we were inconsistent, with NON_NULL used some places, 
NON_EMPTY elsewhere, and no exclusions elsewhere, and in many places 
without much thought.  so we have three options:

1 - dogmatic - remove all `@Include(NON_*)` annotations
2 - compatibility-first - restore anything changed since 0.12.0
3 - forward-looking - leave as is, with documentation added that 
empty/null fields may not be included in json response objects, and 
release note that this has been applied in more places

(note that 1 is technically a breaking change if i had code that 
expected not to see a field unless it was non-empty ... it will probably 
break tests)

i tend to think API consumers impacted by this are small and can take 
the pain, and better to have a nice set of API response objects for new 
users.  and when i see lots of nulls and empties in an api response 
object i think the designers care more about dogma than human users.  i 
don't expect the v2 rest api will land any time soon.

so i am strongly for option 3, taking the ui pain now, but i'll defer if 
i'm a singleton

--a


On 08/11/2017 11:35, Geoff Macartney wrote:
> My tuppence worth - agree with Thomas, it looks like a change best done in
> a V2. Since it's actually a real breakage for the API it would be at least
> worth passing through a deprecation cycle, but as Alex points out there's
> no efficient way to do that. The cost of restoring ugliness in the response
> objects at least keeps the pain away from client API consumers.
>
> G
>
> On Wed, 8 Nov 2017 at 11:22 Graeme Miller <graeme@cloudsoft.io> wrote:
>
>> Hello,
>>
>> I agree with Thomas here. This seems like an API breaking change, and
>> should be reserved for V2 if it at all.
>> I lean towards reverting.
>>
>> Regards,
>> Graeme
>>
>> On 8 November 2017 at 10:01, Thomas Bouron <
>> thomas.bouron@cloudsoftcorp.com>
>> wrote:
>>
>>> Hi all.
>>>
>>> I'm not a fan of excluding fields from the JSON payload, if empty, for
>> few
>>> reasons:
>>> 1. this is a breaking change for the UI and CLI which will be time
>>> consuming to fix (very fiddly to guard against this in JS for example)
>>> 2. this makes it harder to design clients, because you need to guard
>>> against the presence of those fields. The swagger page displays all
>> fields
>>> leading the user/dev to think that they are always returned in the
>> payload
>>> 3. I don't think it saves bandwidth to remove a `constraints: []` from
>> the
>>> final JSON. If you want to have a smaller payload, I would much prefer to
>>> set a query string flag to restrict (or expand) the full body, something
>>> like `?summary=true` => returns only necessary information.
>>>
>>> Now, my comments apply for the API endpoints under /v1 only. I'm all in
>>> favour of break things with a v2 API though.
>>>
>>> Best.
>>>
>>> On Mon, 6 Nov 2017 at 15:17 Alex Heneveld <
>> alex.heneveld@cloudsoftcorp.com
>>> wrote:
>>>
>>>> Hi All-
>>>>
>>>> Until recently our REST API returned full records in most cases,
>>>> including often lots of empty lists and maps and sometimes nulls --
>> such
>>>> as `constraints: []` on all config keys.
>>>>
>>>> The widespread preference in REST / JSON community seems to be to omit
>>>> these unless there is a very good reason for having them, e.g. Google
>>>> JSON Style Guide [1].
>>>>
>>>> Recently in replacing deprecated FasterXML annotations with newer ones
>>>> many fields were changed to be included only if NON_EMPTY, in line with
>>>> that preference, but this is technically a breaking change.  And it's
>>>> not just technical, as there are a few places in UI code which assume
>>>> fields exist which now do break.  These are easy to fix once they're
>>>> encountered at runtime but tedious to ensure no problems at design
>> time.
>>>> Thus we have two choices:
>>>>
>>>> * Yes, make this break now.  This (v1.0) is probably the best time.
>>>> There is no efficient way to pass this through a deprecation cycle.
>> Cost
>>>> is adding to release nodes and REST API client breakages and fixes.
>>>> * No, revert any `Include(NON_EMPTY)` annotations recently introduced
>> to
>>>> be strict about backwards-compatibility here.  Cost is restoring old
>>>> behaviour and bloat/ugliness in the API response objects.
>>>>
>>>> I have a preference for "Yes", roll-forward and tolerate some breakages
>>>> with the shift to 1.0.
>>>>
>>>> Best
>>>> Alex
>>>>
>>>>
>>>> [1]
>>>>
>>>> https://google.github.io/styleguide/jsoncstyleguide.
>>> xml#Empty/Null_Property_Values
>>>> --
>>> Thomas Bouron • Senior Software Engineer @ Cloudsoft Corporation •
>>> https://cloudsoft.io/
>>> Github: https://github.com/tbouron
>>> Twitter: https://twitter.com/eltibouron
>>>


Mime
View raw message