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