search

syndesisio / syndesis-react

[FORK] A flexible, customizable, open source platform that provides core integration capabilities as a service.
Apache License 2.0
3 stars 19 forks source link

fix: api provider bugs #456

Closed riccardo-forina closed 5 years ago

riccardo-forina commented 5 years ago

Partially fixes https://github.com/syndesisio/syndesis/issues/5697 Partially fixes https://github.com/syndesisio/syndesis/issues/5600 Fixes https://github.com/syndesisio/syndesis/issues/5675 Fixes https://github.com/syndesisio/syndesis-react/issues/397 Fixes https://github.com/syndesisio/syndesis/issues/5665

Since the spec is returned by an API, every time you go to the API definition you get the one relative to the last saved version of the integration. So if edit the definition, change your mind and try to edit it again, you'll lose the previous changes.

Also, this will completely delete whatever was present on the integration before the change in the spec. @zregvart can you confirm this is right?

riccardo-forina commented 5 years ago

PR Storybook available here

riccardo-forina commented 5 years ago

PR Storybook available here

pure-bot[bot] commented 5 years ago

Pull request approved by @gashcrumb - applying approved label

zregvart commented 5 years ago

So if edit the definition, change your mind and try to edit it again, you'll lose the previous changes.

I think there's an explicit Save button, so the change your mind bit should be clicking on Cancel, when put this way I think this makes sense.

Also, this will completely delete whatever was present on the integration before the change in the spec. @zregvart can you confirm this is right?

I think in the Angular UI we saved the integration before editing the API definition in Apicurio in that case changes to the flows will be preserved.

riccardo-forina commented 5 years ago

This is an issue, the react ui doesn’t force a save ever, so we can’t guarantee this flow. What would fix this is a version of the api that takes an integration object instead of an integration id, so that we could forward the integration in whatever state it is while the user is working. I guess in this case we would need to save the reference spec somewhere, like in the metadata? This would be to open apicurio directly without asking the be for a spec.

Does this make sense?

zregvart commented 5 years ago

We could do a multipart PUT /api/v1/integrations/{id}/specification, one part Integration JSON another part the new OpenAPI JSON. That should fix it, right?

riccardo-forina commented 5 years ago

Think so but why the id? This could happen for new integrations yet to be saved.

zregvart commented 5 years ago

Think so but why the id? This could happen for new integrations yet to be saved.

That's because we start with the OpenAPI document and create an Integration from it via POST /api/v1/apis/generator that returns the generated Integration JSON. You can't start from both Integration and OpenAPI document...

riccardo-forina commented 5 years ago

Right but again, the integration could not be saved. Anyway, if we store the spec in the metadata we don’t need the specification endpoint anymore, don’t we? We just need the generator one, and that’s the one that should accept both an integration and the spec in the body

zregvart commented 5 years ago

Sure we can do that, there's one caveat POST /api/v1/apis/generator is not without any side effects, we're storing the submitted OpenAPI document, the idea is that we serve from the running integration the same OpenAPI document the client provided.

I'm a bit hesitant to store the OpenAPI specification in metadata as it will be persisted alongside the Integration, and that will be (in general) a large JSON in JSON document which will lead to performance issues.

(we're kinda using the word specification wrong here, it means the OpenAPI document)

I'm thinking we probably need the specification endpoint, but we can create one without side effects. To refactor this fully we would:

  1. rename POST /api/v1/apis/generator to POST /api/v1/integrations/specification to generate integration from specification (receives multipart specification)
  2. rename POST /api/v1/apis/info to POST /api/v1/integrations/specification/info to generate integration summary from specification (receives multipart specification)
  3. add PUT /api/v1/integrations/specification to update the integration from specification (receives multipart specification and integration)
  4. add POST /api/v1/integrations to create integration and specification (receives multipart specification and integration)
  5. remove PUT /api/v1/integrations/{id}/specification

Does that make sense?

riccardo-forina commented 5 years ago

Not sure about 4, doesn't that exist already? It's also a bit problematic to keep hold of the specification for us if it's not stored in the integration itself, the whole editor app is stateless - data gets passed from route to route - and we would have to add the specification string to this passing. It's definitely not ideal.

zregvart commented 5 years ago

I think this discussion just uncovered a bunch of stuff we need discuss and design better, perhaps best done interactively.

I suggest that for the time being we can do the simplest thing, does just adding one endpoint, something like PUT /api/v1/apis/generator that takes both the integration and the OpenAPI document and returns the updated integration based on the OpenAPI document help? It can be invoked as soon as the user saves in Apicurio and the resulting integration can replace the one in the UI?

riccardo-forina commented 5 years ago

I think that would patch this, yep!

zregvart commented 5 years ago

I've created https://github.com/syndesisio/syndesis/issues/5719 and I'll work on it right away.