Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

API Provider Data Type Mismatch #3807

Closed
gaughan opened this issue Oct 11, 2018 · 30 comments · Fixed by #9275
Closed

API Provider Data Type Mismatch #3807

gaughan opened this issue Oct 11, 2018 · 30 comments · Fixed by #9275
Assignees
Labels
cat/bug A bug which needs fixing closed/migrated prio/p1 The priority of a bug. p1 means high source/pm status/never-stale Marker that this issue should not be marked as stale

Comments

@gaughan
Copy link

gaughan commented Oct 11, 2018

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:

image

image

@pure-bot pure-bot bot added notif/triage The issue needs triage. Applied automatically to all new issues. progress/inbox labels Oct 11, 2018
@heiko-braun heiko-braun added cat/bug A bug which needs fixing and removed notif/triage The issue needs triage. Applied automatically to all new issues. labels Oct 11, 2018
@heiko-braun heiko-braun added this to the Sprint 36 (3/4) milestone Oct 11, 2018
@heiko-braun heiko-braun added prio/p1 The priority of a bug. p1 means high and removed progress/backlog labels Oct 11, 2018
@zregvart
Copy link
Member

zregvart commented Oct 12, 2018

@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
Copy link
Author

gaughan commented Oct 12, 2018

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?

@gashcrumb gashcrumb self-assigned this Oct 17, 2018
@KurtStam KurtStam reopened this Oct 18, 2018
@KurtStam
Copy link
Contributor

KurtStam commented Oct 18, 2018

The reason of the mismatch is now more obvious:

screen shot 2018-10-18 at 2 38 14 pm

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
Copy link
Member

  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
Copy link
Contributor

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

@gaughan
Copy link
Author

gaughan commented Oct 18, 2018

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

@zregvart
Copy link
Member

@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
Copy link
Member

@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.

KurtStam pushed a commit to KurtStam/syndesis that referenced this issue Oct 19, 2018
KurtStam pushed a commit to KurtStam/syndesis that referenced this issue Oct 19, 2018
KurtStam pushed a commit to KurtStam/syndesis that referenced this issue Oct 19, 2018
@pure-bot pure-bot bot closed this as completed in #3912 Oct 19, 2018
@pure-bot pure-bot bot locked as resolved and limited conversation to collaborators Oct 19, 2018
@pure-bot pure-bot bot reopened this Oct 19, 2018
@pure-bot pure-bot bot unlocked this conversation Oct 19, 2018
@heiko-braun heiko-braun removed this from the Sprint 36 (3/4) milestone Nov 22, 2018
@heiko-braun
Copy link
Collaborator

cc @syndesisqe Can this be verified?

@tplevko
Copy link
Contributor

tplevko commented Dec 14, 2018

@asmigala can you please provide feedback to eng?

@asmigala
Copy link
Contributor

I no longer see the data mismatch in the Create task operation:
image
This is good

I do see it in the Fetch Task operation:
image
This is also good (the input and output don't match)

I do also see it in Update task:
image
because there's an extra parameters/id variable to be mapped (this corresponds to url parameter defined in swagger):
image
This is probably ok, but I think it could also be mapped automatically.

However, I don't see it in List all tasks:
image
which I think is not ok, because it will result in return nothing.

@KurtStam @zregvart WDYT?

@zregvart
Copy link
Member

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

@asmigala
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Author

gaughan commented Dec 20, 2018

@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
Copy link
Contributor

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
Copy link
Member

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 #3807 (comment).

@gaughan
Copy link
Author

gaughan commented Jan 21, 2019

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

@asmigala
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Member

@asmigala I'll move it to backlog so your last point in #3807 (comment) gets addressed.

@KurtStam KurtStam assigned zregvart and unassigned KurtStam and gashcrumb Mar 25, 2019
@stale
Copy link

stale bot commented Jun 23, 2019

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!

@stale stale bot added the status/stale Issue considered to be stale so that it can be closed soon label Jun 23, 2019
@asmigala asmigala removed the status/stale Issue considered to be stale so that it can be closed soon label Jun 23, 2019
@asmigala asmigala added the status/never-stale Marker that this issue should not be marked as stale label Aug 9, 2019
@heiko-braun
Copy link
Collaborator

zregvart added a commit to zregvart/syndesis that referenced this issue Jan 15, 2021
For cases where action's descriptor has data shape of type 'none' we
would assert that the data shape does not exist. This would cause us not
to recommend adding a mapping step between a input step with data shape
of 'none' and a step containing a data shape other than 'none'. In that
case we do wish to suggest adding a mapping step as mapper can operate
without inputs, i.e. it can add constant values and map those to the
output.

Fixes syndesisio#3807
Ref https://issues.redhat.com/browse/ENTESB-11568
zregvart added a commit to zregvart/syndesis that referenced this issue Jan 18, 2021
For cases where action's descriptor has data shape of type 'none' we
would assert that the data shape does not exist. This would cause us not
to recommend adding a mapping step between a input step with data shape
of 'none' and a step containing a data shape other than 'none'. In that
case we do wish to suggest adding a mapping step as mapper can operate
without inputs, i.e. it can add constant values and map those to the
output.

Fixes syndesisio#3807
Ref https://issues.redhat.com/browse/ENTESB-11568

(cherry picked from commit bd11698)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cat/bug A bug which needs fixing closed/migrated prio/p1 The priority of a bug. p1 means high source/pm status/never-stale Marker that this issue should not be marked as stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants