Syndesis: API Provider Data Type Mismatch

[gaughan]

See also https://issues.jboss.org/browse/ENTESB-11568

This is a...

[ ] Feature request
[ ] Regression (a behavior that used to work and stopped working in a new release)
[x ] Bug report  
[ ] Documentation issue or request

Description

When you provider Todo App swagger to API Provider a data type mismatch occurs on unimplemented flows:

[zregvart]

@gaughan this is due to the fact that the start step has the shape of the request and the end step has the shape of the response. They might differ, and this is quite common.

On this particular case we have a difference due to the same reason behind #3788. The response, or the input shape of the end step, is not wrapped in our _standard_ wrapper for APIs that looks something like:

{
  "parameters": {},
  "body": {}
}

So with #3788 fixed I hope this particular case will not have a mapping difference. Are you concerned about this case (Create new Task operation) that happens to have the same request/response defined in the OpenAPI specification, or are you concerned that the users might end up with data type mismatch warnings in general with API provider-based integrations?

[gaughan]

Thanks @zregvart ,
Yes, my concern is on users ending up with a mismatch in the default case out of the box.
We do all the work to preconfigure the flows but we leave a mismatch as the default.
Maybe we can review after #3788?

[KurtStam]

The reason of the mismatch is now more obvious:

Reason being that there is an extra 'parameters' section on the Target.

So the question now becomes:

1. Do we not worry about it as it maybe obvious to the enduser as to what's going on.
2. Do we equate equality of input and output shapes if the body is identical.
3. Do we try to add the parameters (Content-Type and Status) on the Source. (I don't think this last option makes any sense, though @zregvart is that what you were expecting to happen?)

[zregvart]

1. Do we try to add the parameters (Content-Type and Status) on the Source. (I don't think this last option makes any sense, though @zregvart is that what you were expecting to happen?)

We can totally drop those, I was on the verge of dropping them but I used the fact that they were there. Could be done better...

[KurtStam]

@zregvart ok so we go for option 4 and drop them from the Target side?

[gaughan]

@KurtStam side question, is that icon your own local environment or did we update the atlasmap icon?

[zregvart]

@KurtStam side question, is that icon your own local environment or did we update the atlasmap icon?

@gaughan yup there were a bunch of icon updates in #3842

[zregvart]

@zregvart ok so we go for option 4 and drop them from the Target side?

Yeah, I'd drop them they really have no use and it was silly of me to add them: the status is set on the end step and the Content-Type is determined via the OpenAPI specification. Wasn't very lean of me...

Anyhow now that I've thought of a better way to determine if the data shape is unified or not (via the Syndesis unified JSON schema URN) those are no longer needed.

[heiko-braun]

Kurt, once again, please leave these in the open state, done column. We need this to support the QE workflow

[KurtStam]

@heiko-braun oops - sorry, Any way we can do this via a label? That way I stay in Safari.

[heiko-braun]

For now you would need to look at the history: if pure-bot reopens, please leave it open. In the future I can add a label that matches the zenhub column.

[dongniwang]

Data type mismatch notification is still showing up on the unimplemented flows when using task-api.

I was using the syndesis-1.5.6.fuse-720003 tag.

My concern is that users are drawn to the orange notificaiton and they will want to resolve the warning by adding a data mapping step, which is perhaps not neccessary based on my understanding.

See screenshot below:

[KurtStam]

@dongniwang can you bring up the data mapper? It will show you why they are different. Depending on how the API was defined the IN and OUTPUT may not be the same on purpose. I think the API you used only takes one field 'body' as input while it outputs the entire Todo object. So it would be a valid requirement to put in a mapper.

[KurtStam]

Ready for QE

[dongniwang]

@KurtStam - OK. That makes sense. But then I was wondering if we can display the input and output types more obviously in the UI and help users make the understand that the Data type mismatch is because of the differences in the input and output types.

What would happen if users first add a data mapping step (as the result of seeing the data type mismatch notification), then add more steps or connections in the flow (either before or after the data mapping step they just added)?

[heiko-braun]

cc @syndesisqe Can this be verified?

[tplevko]

@asmigala can you please provide feedback to eng?

[asmigala]

I no longer see the data mismatch in the Create task operation:

This is good

I do see it in the Fetch Task operation:

This is also good (the input and output don't match)

I do also see it in Update task:

because there's an extra parameters/id variable to be mapped (this corresponds to url parameter defined in swagger):

This is probably ok, but I think it could also be mapped automatically.

However, I don't see it in List all tasks:

which I think is not ok, because it will result in return nothing.

@KurtStam @zregvart WDYT?

[zregvart]

@asmigala I think the list all tasks has no input shape and that's why there is no warning there.

[asmigala]

@zregvart right, but is that the expected behaviour? The orange triangle should advise the user that the route in the current state will not return any data unless they do something about it (i.e. add a data mapper). Now I will concede that returning an empty array would be ok here, but what happens in reality is that the response is completely empty.
Not sure this is what we want.

[zregvart]

@zregvart right, but is that the expected behaviour?

I'd say that for any data shape mismatch the user should be warned. That is _no data shape_ is not matched by _some data shape_. In this specific case perhaps we need a _void_ datashape on the start (the { api } thinggy), that could provoke the data shape mismatch on the return path.

WDYT @gaughan?

[gaughan]

@zregvart I agree, we want to give as much help to enable the user to successfully deploy.
I really like the map automatically suggestion. Automatic config where reasonable would greater improve OOTB experience.

[asmigala]

What is the status of this issue? Are the concerns mentioned in my comment above going to be addressed, or should I close as is and open a new issue for those concerns?

[zregvart]

My 2c is that we should create a new issue(s) for refinements. Skimming through seems that we have just one case that we should handle better; the one outlined in https://github.com/syndesisio/syndesis/issues/3807#issuecomment-448948660.

[gaughan]

@zregvart I think we should keep this in mind with other API provider enhancements, a more seamless UX is ideal

[asmigala]

@zregvart I think this is still the same issue ("When you provider Todo App swagger to API Provider a data type mismatch occurs on unimplemented flows" as in the original report).

I am ok with closing this and opening a new issue, as this in particular is fairly minor issue, but I think it sets a dangerous precedent on what it means an issue is "fixed".

[asmigala]

@zregvart @gaughan have we reached a decision regarding this issue? Current master still does not have the warning as mentioned in my previous comments.

[zregvart]

@asmigala I'll move it to backlog so your last point in https://github.com/syndesisio/syndesis/issues/3807#issuecomment-447815848 gets addressed.

[stale[bot]]

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

[heiko-braun]

Migrated to https://issues.jboss.org/browse/ENTESB-11568