That should _not_ be allowed, it must be a dict/object. No array, or
other primitive at the top level.
-ash
On May 29 2020, at 3:27 pm, Tomasz Urbaszek <turbas...@apache.org> wrote:
> Do I correctly understand that users can specify conf="[1,2,3]" as
> this is
> also a valid JSON?
>
> T.
>
> On Fri, May 29, 2020 at 4:18 PM Kamil Breguła <kamil.breg...@polidea.com>
> wrote:
>
>> The problem is with mixing objects that have a known structure with
>> objects that do not have this structure. Common JSON parser in Java
>> can generate objects if all the attributes have a known type.
>> Attributes that are unrecognized are ignored because Java cannot
>> dynamically generate classes. This problem is very visible when
>> reading a list of objects from the API.
>>
>> For example. We have the following DAGRun.
>> DagRun(conf='{"A":"B"}')
>> DagRun(conf='{"C":"D"}')
>> They can be represented by the following JSON objects.
>> {"conf": {"A":"B"}}
>> {"conf": {"C":"D"}}
>>
>> A type representing the DAG Run is automatically generated based on
>> the API specification. I prepared an example that shows such a type in
>> many different technologies for AJava
>> I used official library generators for OpenAPI
>> https://github.com/OpenAPITools/openapi-generator
>> Example class definitions
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-okhttp-gson/src/main/java/org/openapitools/client/model/DAGRun.java#L64
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-feign/src/main/java/org/openapitools/client/model/DAGRun.java#L65
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-jersey1/src/main/java/org/openapitools/client/model/DAGRun.java#L65
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-jersey2/src/main/java/org/openapitools/client/model/DAGRun.java#L65
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-microprofile/src/main/java/org/openapitools/client/model/DAGRun.java#L64
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-native/src/main/java/org/openapitools/client/model/DAGRun.java#L65
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-okhttp-gson/src/main/java/org/openapitools/client/model/DAGRun.java#L63
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-rest-assured/src/main/java/org/openapitools/client/model/DAGRun.java#L64
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-resteasy/src/main/java/org/openapitools/client/model/DAGRun.java#L65
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-resttemplate/src/main/java/org/openapitools/client/model/DAGRun.java#L65
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-retrofit2/src/main/java/org/openapitools/client/model/DAGRun.java#L63
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-vertx/src/main/java/org/openapitools/client/model/DAGRun.java#L65
>>
>> https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/java-webclient/src/main/java/org/openapitools/client/model/DAGRun.java#L64
>> None allowed to generate a class structure that allows reading the
>> attributes contained in the conf field if type == object.
>>
>> Libraries for other languages and with different fields are also
>> available in the repository.
>>
>> As an object:
>> Patch:
>> https://github.com/mik-laj/airflow-api-clients/blob/master/spec-object.patch
>> Libraries:
>> https://github.com/mik-laj/airflow-api-clients/tree/master/out/object
>>
>> Two field types at the same time:
>> Patch:
>> https://github.com/mik-laj/airflow-api-clients/blob/master/spec-oneoff.patch
>> Libraries:
>> https://github.com/mik-laj/airflow-api-clients/tree/master/out/oneoff
>>
>> Without modification (string):
>> Libraries:
>> https://github.com/mik-laj/airflow-api-clients/tree/master/out/string
>>
>>
>> I have the biggest concerns about reading data, not about sending data
>> so in my opinion we should think about how we want the response from
>> the server to look like?
>>
>> GET //dags/{DAG_ID}/dagRuns/
>>
>> {
>> "dag_runs": [
>> {"conf": {"A":"B"}}
>> {"conf": {"C":"D"}}
>> ]
>> }
>>
>> or
>> "dag_runs": [
>> {"conf": "{\"A\":\"B\"}}
>> {"conf": "{\"C\":\"D\"}}
>> ]
>> }
>>
>> If we choose one output format, we can think about whether we want to
>> make changes to this object structure so that the input object does
>> not match the output object. In my opinion, this is not a good idea.
>> REST is based on resources/types that have a unified interface and
>> fields that are ambiguous can be problematic for users.
>>
>> In my opinion, the problem is that the field format and the message
>> format is JSON. This is just a coincidence. In the future, we may add
>> a different message format, e.g. protobuff, and we will not be able to
>> provide common schema. We will have to introduce discrepancies between
>> spec for protobuff and spec for JSON when we change only transform
>> format.
>>
>> On Fri, May 29, 2020 at 12:08 PM Ash Berlin-Taylor <a...@apache.org> wrote:
>> >
>> > On May 29 2020, at 8:29 am, Jarek Potiuk <jarek.pot...@polidea.com>
>> wrote:
>> > > *Root problem*
>> > >
>> > > I think the root problem is not in the interface/API but the ambiguity
>> of
>> > > the DagRun.conf field. The way it is defined now, It can actually
>> be an
>> > > arbitrary object, not even a dictionary.
>> >
>> > So that is a gap in the definition. It _needs_ to be a dictionary/JSON
>> > *Object* specifically, else it will break when it is used.
>> >
>> > > but in case of the API, it is just a tool that is used to pass the
>> > > data, and there should be some well-defined and fixed logic about the
>> > > structure
>> >
>> > My view is that specifying it as a JSON object in the spec (but saying
>> > nothing about the properties it might contain) is more precise than a
>> > string. Compare these two snippets of OpenAPI spec:
>> >
>> >
>> > conf:
>> > type: string
>> > description: >
>> > JSON string (which must contain a top-level JSON object)
>> > describing additional configuration parameters.
>> >
>> > vs
>> >
>> > conf:
>> > type: object
>> > description: >
>> > JSON object describing additional configuration parameters.
>> >
>> >
>> > 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.
>> >
>> > (With the string case, we would have to JSON deserialize it and check
>> > it's an JSON object/ python dict. Nothing else is allowed.)
>> >
>> > > *Java case (and other static-typed languages)*
>> >
>> > import com.google.gson.Gson;
>> > import com.google.gson.JsonObject;
>> >
>> > class Demo {
>> >
>> > public static class Employee
>> > {
>> > private Integer id;
>> > private String firstName;
>> > private String lastName;
>> >
>> > public Employee(Integer id, String firstName, String lastName){
>> > this.id = id;
>> > this.firstName = firstName;
>> > this.lastName = lastName;
>> > }
>> > }
>> >
>> > public static void main(String args[]) {
>> >
>> > Demo.Employee employee = new Demo.Employee(1, "Ash",
>> "Berlin-Taylor");
>> >
>> > Gson gson = new Gson();
>> >
>> > JsonObject e = gson.toJsonTree(employee).getAsJsonObject();
>> >
>> > // Just for debugging/testing
>> > System.out.println(e.toString());
>> >
>> > }
>> > }
>> >
>> > which outputs
>> >
>> > {"id":1,"firstName":"Ash","lastName":"Berlin-Taylor"}
>> >
>> >
>> > Entirely possible to create arbitrary JSON structures in Java. (You can
>> > also just create a com.google.gson.JsonObject directly and call `.add()`)
>> >
>> > 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.
>> >
>> >
>> > So I'm still +1 for dict directly. Even more so now I have written this
>> > in Java.
>> >
>> > JSON-encoded-string in a JSON api is a "code smell" to me.
>> >
>> > -ash
>> >
>> >
>> > On May 29 2020, at 8:29 am, Jarek Potiuk <jarek.pot...@polidea.com>
>> wrote:
>> >
>> > > And one more comment to add - this is the DagRun class I wrote
>> for the
>> > > imaginary Java client. That's all (including the serializer from the
>> > > previous mail) needed to be able to easily build the POST method call
>> in
>> > > Java.
>> > > I would really like to challenge someone more experienced with Java
>> writes
>> > > or provides some examples, either the class holding arbitrary complex
>> conf
>> > > object or (if we stick to String way of storing the String) - to
>> > > serialize/deserialise the object from son.
>> > >
>> > > I really believe it is far from trivial and by choosing "object"
>> way of
>> > > sending the conf, we significantly increase the complexity of clients
>> > > accessing Airflow API (which should be our goal) at the expense of
>> > > "slightly" less readable code.
>> > >
>> > > J.
>> > >
>> > >
>> > > public static class DagRun {
>> > > private final String dagRunId;
>> > > private final String executionDate;
>> > > private final String conf;
>> > >
>> > > public String getDagRunId() {
>> > > return dagRunId;
>> > > }
>> > >
>> > > public String getExecutionDate() {
>> > > return executionDate;
>> > > }
>> > >
>> > > public String getConf() {
>> > > return conf;
>> > > }
>> > >
>> > > public DagRun(String dagRunId, String executionDate, String
>> conf){
>> > > this.dagRunId = dagRunId;
>> > > this.executionDate =executionDate;
>> > > this.conf = conf;
>> > > }
>> > > }
>> > >
>> > > J.
>> > >
>> > >
>> > > On Fri, May 29, 2020 at 8:40 AM Jarek Potiuk <jarek.pot...@polidea.com
>> >
>> > > wrote:
>> > >
>> > >> After giving it quite some time to try and think about it this
>> morning,
>> > >> and looking at consequences - I am in strong favour of passing string
>> as
>> > >> conf (Kamil's proposal).
>> > >>
>> > >> I don't think the dictionary is good. And trying to accommodate both
>> > >> is I
>> > >> think combining the worst of both worlds. Let me explain why.
>> > >>
>> > >> *Root problem*
>> > >>
>> > >> I think the root problem is not in the interface/API but the
>> > >> ambiguity of
>> > >> the DagRun.conf field. The way it is defined now, It can
>> actually be
>> an
>> > >> arbitrary object, not even a dictionary. I could pass it an
>> array, or
>> > >> bool, or pretty much any serializable object it seems. Except
>> for a
>> few
>> > >> tests where both "string" and "dict" are accepted in the old
>> experimental
>> > >> API I do not see anywhere (please correct me if I am wrong) any kind
>> of
>> > >> specification for the conf object. That indeed makes it rather
>> hard to
>> > >> reason about it in statically typed languages. And while I understand
>> why
>> > >> we need it and why in Python environment we are fairly relaxed about
>> this
>> > >> (JINJA for example) - I do not think this should influence the
>> complexity
>> > >> of the API "structure".
>> > >>
>> > >> *API Structure: *
>> > >>
>> > >> I think JSON structure in the API should be fixed and well
>> defined. I
>> > >> think it is much better to be very explicit about the "conf"
>> > >> parameter that
>> > >> it is "string JSON representation" than the arbitrary object. JSON
>> > >> is, of
>> > >> course, dynamic but in case of the API, it is just a tool that is
>> > >> used to
>> > >> pass the data, and there should be some well-defined and fixed logic
>> about
>> > >> the structure.Precisely to make it easier to parse and prepare. I
>> > >> have a
>> > >> feeling that putting that dynamic nature of JSON into API structure
>> > >> definition is quite an abuse of that dynamic nature.
>> > >>
>> > >> *Java case (and other static-typed languages)*
>> > >>
>> > >> I haven't written Java for years, but I decided to give it a
>> try. I
>> tried
>> > >> to see how complex it would be to write serialization/deserialization
>> for
>> > >> that using one of the most common parsers in Java world - Gson.
>> > >>
>> > >> The string case is super simple:
>> > >>
>> > >> public JsonElement serialize(final DagRun dagRun, final Type
>> type,
>> > >> final JsonSerializationContext context) {
>> > >> JsonObject result = new JsonObject();
>> > >> result.add("dag_run_id", new
>> > >> JsonPrimitive(dagRun.getDagRunId()));
>> > >> result.add("execution_date", new
>> > >> JsonPrimitive(dagRun.getExecutionDate()));
>> > >> result.add("conf",new JsonPrimitive(dagRun.getConf()));
>> > >> return result;
>> > >> }
>> > >>
>> > >> The dynamic object of arbitrary complexity I do not even know
>> how to
>> > >> approach. Eventually what I would have to do is to convert it to JSON
>> > >> String anyway - because that's the only way you can keep arbitrary
>> complex
>> > >> structure in Java.
>> > >>
>> > >> *Deferring problem?*
>> > >>
>> > >> Also - I do not think we are deferring the problem for later. The
>> > >> thing is
>> > >> that the only entity that cares about the "content" of the conf being
>> > >> accessible as an object is the Python DAG reading the conf object
>> (likely
>> > >> with some JINJA templates).
>> > >>
>> > >> We will likely never, ever have to parse, de-jsonize the string on
>> the
>> > >> client-side. We will just have to prepare it (to send) and possibly
>> display
>> > >> it (if we ever read that conf via API).
>> > >> The imaginary client communicating with Airflow will simply pass
>> whatever
>> > >> the User will tell it to do. And IMHO this is far easier if we
>> do not
>> have
>> > >> to convert it to object on the flight .
>> > >>
>> > >> I can imagine several use cases for that method:
>> > >>
>> > >> 1) User types - by hand - the whole "conf" object to pass to the
>> trigger
>> > >> method. Likely typing JSON-string directly. That's a typical
>> case for
>> any
>> > >> kind of CLI I can imagine - where usually you pass JSON string for
>> > >> arbitrary complex objects.
>> > >>
>> > >> 2) The client-side implementation will define some limited set of
>> > >> parameters that could be used by the user and let the user enter it
>> > >> via a
>> > >> form. Based on that a "conf" object will be created following
>> predefined
>> > >> structure (specific for this case). For example, the user
>> chooses a
>> > >> date to
>> > >> run, and the form produces {"date: "${DATE_CHOSEN_BY_THE_USER}" }
>> object.
>> > >>
>> > >> 3) Theoretically, it's possible that user enters arbitrary complex
>> OBJECT
>> > >> via any kind of structured "generic" interface. That would be a
>> nightmare,
>> > >> however (both from the user and developer point of view). So I
>> disregard
>> > >> that option.
>> > >>
>> > >> In case of 1) we would have to parse the data ??? and turn it into
>> > >> object to serialize. Python can do that, but Java can't easily. And
>> > >> well -
>> > >> there is no point in doing it - we cannot do anything with conf as
>> object,
>> > >> we have no idea if it is valid or not, we have to anyhow pass it to
>> > >> DAG so
>> > >> that DAG can access whatever fields are needed. I think we would be
>> better
>> > >> to pass it as the very string user entered (After we check if
>> this is
>> a
>> > >> valid json - which we can do easily).
>> > >>
>> > >> In case of 2) it's also super easy to turn such pre-defined structure
>> into
>> > >> JSON string. It's trivial in any language. There is virtually no
>> > >> benefit of
>> > >> passing it as object. - the "slightly" better readabilty maybe the
>> > >> only one
>> > >>
>> > >> Of course - as Ash and Kaxil mentioned, we could pass both -
>> string or
>> > >> dict, but I think that is very wrong. Should we also allow array
>> > >> (this is a
>> > >> valid json structure)? Should we allow passing standalone bool, int
>> ..
>> > >> objects (they are valid json). Also how about sending "STRING" as
>> conf
>> > >> value (is it string or is it json-encoded object). This is a
>> bad, bad
>> idea.
>> > >>
>> > >> So summarizing - I am strongly for passing "string" rather than
>> object.
>> > >>
>> > >> J.
>> > >>
>> > >>
>> > >> On Fri, May 29, 2020 at 1:58 AM Kaxil Naik <kaxiln...@gmail.com>
>> wrote:
>> > >>
>> > >>> If the only problem is with Java and not any other popular
>> > >>> languages, I
>> > >>> would say we go for Option (2).
>> > >>>
>> > >>> If not, supporting both is a good idea.
>> > >>>
>> > >>> Regards,
>> > >>> Kaxil
>> > >>>
>> > >>> On Fri, May 29, 2020 at 12:19 AM QP Hou <q...@scribd.com> wrote:
>> > >>>
>> > >>> > While I understand the difficulty of dealing with nested json
>> without
>> > >>> > predefined schemas, I feel like returning it as a string only
>> delays
>> > >>> > the problem to a later stage since the user will still need to
>> parse
>> > >>> > that string into a strongly typed data structure in order to read
>> the
>> > >>> > values.
>> > >>> >
>> > >>> > I don't have much experience in Java so I can't really
>> comment on
>> > >>> > that. But I can confirm that it's pretty straightforward to deal
>> with
>> > >>> > this in C/C++, Rust and Go.
>> > >>> >
>> > >>> > On Thu, May 28, 2020 at 2:57 PM Ash Berlin-Taylor <a...@apache.org>
>> > >>> wrote:
>> > >>> > >
>> > >>> > > Hi everyone,
>> > >>> > >
>> > >>> > > We're really close to getting the OpenAPI spec merged, just one
>> last
>> > >>> > > question that's come up around how we should handle/represent
>> > >>> > > dagrun.conf to triggerDagRun.
>> > >>> > >
>> > >>> > > Which of the these two do people prefer?
>> > >>> > >
>> > >>> > >
>> > >>> > > POST /api/v1/dags/{dag_id}/dagRuns/{dag_run_id}
>> > >>> > > Content-Type: application/json
>> > >>> > >
>> > >>> > > {
>> > >>> > > "dag_run_id": "manual_2020-05-28T21:42:36Z",
>> > >>> > > "execution_date": "2020-05-28T21:42:36Z",
>> > >>> > > "conf": "{\"key\": \"value\" }"
>> > >>> > > }
>> > >>> > >
>> > >>> > > OR
>> > >>> > >
>> > >>> > >
>> > >>> > > POST /api/v1/dags/{dag_id}/dagRuns/{dag_run_id}
>> > >>> > > Content-Type: application/json
>> > >>> > >
>> > >>> > > {
>> > >>> > > "dag_run_id": "manual_2020-05-28T21:42:36Z",
>> > >>> > > "execution_date": "2020-05-28T21:42:36Z",
>> > >>> > > "conf": {"key": "value"}
>> > >>> > > }
>> > >>> > >
>> > >>> > > i.e. should the schema/type of conf be a (JSON-encoded) string,
>> > >>> or an
>> > >>> > object.
>> > >>> > >
>> > >>> > > I favour the later, Kamil the former. His point is that staticly
>> typed
>> > >>> > > languages, and Java in particular, would be hard to represent
>> this.
>> > >>> > > (Please correct me if I've over-simplified or misunderstood your
>> > >>> > > argument Kamil)
>> > >>> > >
>> > >>> > > Mine was that it's easy enough in Go, for example
>> trigger(dagRunId
>> > >>> str,
>> > >>> > > executionDate *time.Time, conf interface{})`, and double json
>> encoding
>> > >>> > > is always messy/a pain to drive manually on cURL etc.
>> > >>> > >
>> > >>> > >
>> > >>> > > (Using dagRun.conf is quite rare right now, doubly so via the
>> > >>> API, so
>> > >>> I
>> > >>> > > don't think we have any precendent to follow.)
>> > >>> > >
>> > >>> > > Or does anyone feel strongly that we should support both, and
>> have
>> > >>> this
>> > >>> > > in the python side
>> > >>> > >
>> > >>> > > if conf:
>> > >>> > > if isinstance(conf, dict):
>> > >>> > > run_conf = conf
>> > >>> > > else:
>> > >>> > > run_conf = json.loads(conf)
>> > >>> > >
>> > >>> > >
>> > >>> > > -ash
>> > >>> >
>> > >>>
>> > >>
>> > >>
>> > >> --
>> > >>
>> > >> Jarek Potiuk
>> > >> Polidea <https://www.polidea.com/> | Principal Software Engineer
>> > >>
>> > >> M: +48 660 796 129 <+48660796129>
>> > >> [image: Polidea] <https://www.polidea.com/>
>> > >>
>> > >>
>> > >
>> > > --
>> > >
>> > > Jarek Potiuk
>> > > Polidea <https://www.polidea.com/> | Principal Software Engineer
>> > >
>> > > M: +48 660 796 129 <+48660796129>
>> > > [image: Polidea] <https://www.polidea.com/>
>> > >
>>
>
