@Test
public void confDynamicTest() {
JsonObject x = new JsonObject();
x.addProperty("x", 34);
model.setConf(x);
Assert.assertEquals(new JSON().serialize(model),
"{\"external_trigger\":true,\"conf\":{\"x\":34}}");
JsonObject a = new JsonObject();
JsonArray aa = new JsonArray();
aa.add(1);
aa.add(2);
aa.add(3);
a.add("a", aa);
model.setConf(a);
Assert.assertEquals(new JSON().serialize(model),
"{\"external_trigger\":true,\"conf\":{\"a\":[1,2,3]}}");
JsonObject zw = new JsonObject();
JsonArray z = new JsonArray();
z.add(1);
z.add(2);
zw.add("Z", z);
zw.addProperty("W", "a");
model.setConf(zw);
Assert.assertEquals(new JSON().serialize(model),
"{\"external_trigger\":true,\"conf\":{\"Z\":[1,2],\"W\":\"a\"}}");
}
All tests passing.
On May 29 2020, at 9:17 pm, Jarek Potiuk <jarek.pot...@polidea.com> wrote:
> Ash - I'd really love to see your example extended to handle dynamic
> objects:
>
>
> a)
> conf : {
> "x": 34
> }
>
> b)
> conf: {
> "a": [1, 2, 3]
> }
>
> c)
>
> conf: {
> "Z": [1,2],
> "W": "a"
> }
>
>
> Without having to recompile the java client every time a new structure of
> conf is added.
>
> J,
>
>
>
>
> On Fri, May 29, 2020 at 10:13 PM Ash Berlin-Taylor <a...@apache.org> wrote:
>
>> Responses inline
>>
>>
>>
>>
>> On May 29 2020, at 5:49 pm, Jarek Potiuk <jarek.pot...@polidea.com> wrote:
>>
>>
>> > From what I understand - that single choice prevents our customers to
>> > use auto-generation in many languages
>> > = reasons for that explained by Kamil). Here are few examples:
>> >
>> > * Typescript:
>> https://www.gitmemory.com/issue/OpenAPITools/openapi-generator/4119/540506783
>>
>> > * Java: https://github.com/OpenAPITools/openapi-generator/issues/2037
>>
>> This issue has been fixed for over a year.
>>
>> > * REST client for JIRA (not sure which language) :
>> >
>> https://community.developer.atlassian.com/t/jira-rest-client-generation-fails-swagger-open-api-generator/36398
>>
>> > * GO:
>> https://medium.com/real-apis/free-form-objects-with-array-values-72e85c044e4a
>>
>> Talks very specificazlly about Arrays, not objects, so I don't think
>> this applies.
>>
>> >
>> > The problem is that the right code cannot be generated for those
>> > languages for freeForm
>> > unless you specify "additionalProperties" field which will tell at
>> > least what types the keys and values
>> > have. Which we cannot do in our case.
>>
>> `additionalProperties: true` is supported by the OpenAPI spec to say
>> "anything else is allowed.
>>
>>
>> >
>> > Unless someone proves that we can generate automatically, usable
>> > clients in those
>> > languages automatically this one will be vetoed from my side.
>>
>>
>> Using's Kamil's generated Java client (with deleting the broken
>> references to OneOfScheduleInterval as it isn't relevant to this point),
>> my same Employee example from earlier.
>>
>>
>> import org.openapitools.client.JSON;
>>
>> ...
>>
>> @Test
>> public void confTest() {
>> // TODO: test conf
>> //
>> Employee employee = new DAGRunTest.Employee(1, "Ash",
>> "Berlin-Taylor");
>> model.setConf(employee);
>>
>> Assert.assertEquals(new JSON().serialize(model),
>>
>> "{\"external_trigger\":true,\"conf\":{\"id\":1,\"firstName\":\"Ash\",\"lastName\":\"Berlin-Taylor\"}}");
>> }
>>
>> That passes. So it's trivially easy on the setting side. Working out the
>> reading deser. side now.
>>
>> -ash
>>
>>
>> >
>> > I think the ease-of-use and the Open API specification we have is to
>> > make it possible
>> > for the users to use the API easily. but in this case, by one line
>> > decision we are basically
>> > forcing our users to hand-write their whole code
>> > instead of generating it all because of one "FreeForm" object in
>> the API.
>> >
>> > I don't think there is a lot of empathy to our users if we make this
>> decision.
>> > To be honest I see it as a bit selfish decision (if what I understand
>> > is correct).
>> >
>> >> The first one means technically, acording to the spec you can
>> submit `{
>> >> "conf": "abc" }` -- but that will fail. Where as the second means that
>> >> both the OpenAPI client, and the automatic validation from
>> Connexion on
>> >> the server will handle that for us.
>> >
>> > Not fully really because in at least few languages the OpenAPI client
>> > - and validation -
>> > will have to be manually written anyway.
>> >
>> >> (With the string case, we would have to JSON deserialize it and check
>> >> it's an JSON object/ python dict. Nothing else is allowed.)
>> >
>> > Yeah. Now that we know that it's not allowed we know it needs to be
>> dictionary.
>> > Indeed - we will have to validate that ourselves. There is no
>> > performance loss though
>> > more security (open API has to do it anyway while parsing for "object
>> > type" and does not
>> > have to do it for string)
>> >
>> > As I've written before - the only real validation of the data is done
>> > by the
>> > DAG reading the values. Nothing prevents the user from sending { "a":
>> > {"b" : [] } },
>> > when dag expects {"a": "b"}. We cannot *really* validate the data with
>> > Open API.
>> >
>> > And I think single `isinstance(json.loads(conf),dict)` is far, far, far
>> > much easier to write than hand-coding your client class instead of
>> > generating it.
>> >
>> > As I see it now in this case, the real trade-off we are dealing with
>> > here is:
>> >
>> > * "should we add this line of code to validate" vs.
>> > * "should we make our users suffer and spend extra xx days to write
>> their
>> > client libraries instead of generating them".
>> >
>> >> To use your example DagRun class, the signature could become:
>> >>
>> >> public DagRun(String dagRunId, String executionDate, Object conf);
>> >>
>> >> and then the client can do `gson.toJsonTree(conf).getAsJsonObject()`
>> internally.
>> >
>> > Actually - It does not work that easily with the dynamic Objects and
>> > structure of the objects.
>> > If you want to pass conf as the conf object to the class and get it
>> working
>> > as you described, you will have to define a separate class for EVERY
>> > TYPE of conf object
>> > that you pass to different DAG and know which of the Type object
>> > should be passed
>> > to the DagRun constructor.
>> >
>> > This basically means that if you have two DAGs with two different set
>> > of config parameters you need to define
>> > different Class for each DAG and know which class shoudl be passed to
>> > the constructor
>> > Basically this means that the users will have to develop, recompile
>> > and release Java client
>> > every time they add a new DAG that might require some configuration.
>> >
>> > With the string solution the natural thing will be that conf is
>> > hand"coded" and generated dynamically rather
>> > then recompiled as a new Java class.
>> >
>> > And you have not shown how you would convert the "client" case I
>> > mentioned where you specify the conf object
>> > via command line as string. How would you decide which class to use?
>> > Based on what? How do you know
>> > which DAG is which conf class? I think this is a really difficult one
>> > to pull by the client if they use
>> > statically typed language like Java.
>> >
>> >> JSON-encoded-string in a JSON api is a "code smell" to me.
>> >
>> > I think we are giving our static language client users much more than
>> > a "smell" here.
>> >
>>
>
>
> --
>
> Jarek Potiuk
> Polidea <https://www.polidea.com/> | Principal Software Engineer
>
> M: +48 660 796 129 <+48660796129>
> [image: Polidea] <https://www.polidea.com/>
>
