On Fri, May 29, 2020 at 7:18 AM Kamil Breguła <kamil.breg...@polidea.com> wrote:
> 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.
Again, back to my previous comment, I feel like by encoding it as a
string, we are just pushing this problem to a different part of the
code base. The user still needs to serialize that string into a
generically typed object in order to use it. If the user doesn't need
to read/use conf attribute, then I think it doesn't matter if it is a
string or generic object or not, it will just be passed along as is.
Defining the field as a generic object has another benefit of catching
type errors at build time. if it's a string, the user could pass in
invalid json strings like "{]" to the client and the compiler will
happily accept it. I don't know how the Object type in Java works, but
for the golang example you linked, the compiler can at least guarantee
the client will be sending a map that can be serialized to a valid
json at build time.
> 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.
Compatibility with other serialization formats is a good point to
bring up. For protobuf specifically, I think this can be represented
using Struct
message type [1][2]. It will be worth looking into other popular
serialization formats we want to support in the future.
As of today, the protobuf codegen in openapi-generator has not
implemented support for object type yet:
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/ProtobufSchemaCodegen.java#L152.
This will require extra work to add support to openapi-generator, but
it's definitely doable.
> 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
This looks like a implementation error of openapi-generator, not an
issue with free form object pattern itself. As mentioned in the
comment, it should be implemented the same way the go client generator
is, which is to model it as a map with string as key and generic
object as value.
> * Java: https://github.com/OpenAPITools/openapi-generator/issues/2037
This looks like another openapi-generator implementation issue that's
already fixed?
> * REST client for JIRA (not sure which language) :
> https://community.developer.atlassian.com/t/jira-rest-client-generation-fails-swagger-open-api-generator/36398
Based on pasted logs in that thread, it looks like another
openapi-generator implementation error to me.
> * GO:
> https://medium.com/real-apis/free-form-objects-with-array-values-72e85c044e4a
Based on the generated go client code that Kamil linked above, it's
already using the right type for conf [3], which is
"map[string]interface{}" as mentioned in the blog post.
On top of this, the example used in this blog post is different
because it actually has a strict schema for each value item, which is
id in string. In our case, the value is a truly free-formed nested
JSON object.
> 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.
Again, I am not familiar with Java, but I can confirm that you won't
have to do this for languages like Go, Rust and C/C++ :) I have feel
that if it works in Rust, then it should work for Java as well since
Java has a more dynamic runtime.
Basically what I am trying to say is from the perspective of language
design, I can't think of any reason why static typing will make free
formed json objects hard to work with. Static typed languages can
still use runtime flection to handle cases like this. There could be
some implementation issues in openapi-generator that could cause
issues down the road, which will require sending upstream patches.
So the question to us is whether we want to spend time improving
openapi-generator down the road, if not, then we should just use
string type instead of object to work around all the potential
implementation issues.
[1]:
https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/struct.proto#L51
[2]: https://groups.google.com/forum/#!topic/protobuf/ZWp_REiHlF4
[3]:
https://github.com/mik-laj/airflow-api-clients/blob/master/out/object/go/model_dag_run.go
