Mailing-List: contact dev-help@brooklyn.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@brooklyn.apache.org
From: Alex Heneveld <alex.heneveld@cloudsoftcorp.com>
Subject: Re: Brooklyn REST API - omitting fields in JSON objects
To: dev@brooklyn.apache.org
References: <ffa823a3-c96c-23f1-cd5e-b202d243687a@cloudsoft.io>
 <CAH20XTaouztMfou9cfoUuh+oJepn-PVKjbUAwUtGjK5vF2Oh6A@mail.gmail.com>
 <CAKEvtrR8HF9XLGK19EyV80E9cfSREqLxH9AdrYk+fTO1tG1xTg@mail.gmail.com>
 <CAE7C0PcHcuSB1ekzYvq__25iO+aMNYFfMQNUdjfR35PhxzdSOA@mail.gmail.com>
Message-ID: <4fd6ffc8-5092-d758-6e04-7148b4233a6e@CloudsoftCorp.com>
Date: Wed, 8 Nov 2017 12:24:06 +0000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0)
 Gecko/20100101 Thunderbird/45.5.1
MIME-Version: 1.0
In-Reply-To: <CAE7C0PcHcuSB1ekzYvq__25iO+aMNYFfMQNUdjfR35PhxzdSOA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
archived-at: Wed, 08 Nov 2017 12:24:20 -0000


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
>>>