appsmithorg / appsmith

Platform to build admin panels, internal tools, and dashboards. Integrates with 25+ databases and any API.
https://www.appsmith.com
Apache License 2.0
34.35k stars 3.72k forks source link

[Bug] Saving a datasource does not update actions #2373

Open Nikhil-Nandagopal opened 3 years ago

Nikhil-Nandagopal commented 3 years ago

Description

On adding bindings to a datasource and saving it, the same is not reflected in the actions related to the datasource till the page is refreshed

Steps to reproduce the behaviour:

  1. Create an API and save a datasource against it
  2. Navigate to the datasource and add a binding in the header. ex. token: {{appsmith.store.token}}
  3. save the datasource
  4. Inspect the network
  5. navigate to the API and run it.
  6. The API runs without the binding
  7. Refresh the page
  8. Navigate to the API and run it.
  9. The API runs with the binding

The API should run with the binding in both situations. This is happening because the action is updated on the server when the datasource is saved but not on the client

sumitsum commented 3 years ago

As per my investigation, action is not updated in the db with the header key value pairs when the API datasource is saved with header info. Removing the front-end tag.

Nikhil-Nandagopal commented 3 years ago

@sumitsum if that is the case why does refreshing the page work?

sumitsum commented 3 years ago

@Nikhil-Nandagopal it didn't work for me. Please check the video and see if I am looking at the right problem:

https://user-images.githubusercontent.com/1757421/113002469-6d989880-918f-11eb-876c-0b113b4ae69b.mov

Also, a snapshot from my local db:

db.datasource.find({'name':'Api1Ds'}) { "_id" : ObjectId("606323c27f7ba17ce75dbe07"), "name" : "Api1Ds", "pluginId" : "5ca385dc81b37f0004b4db85", "organizationId" : "5f9fc0550b682b2a63e424d3", "datasourceConfiguration" : { "sshProxyEnabled" : false, "properties" : [ { "key" : "isSendSessionEnabled", "value" : "N" }, { "key" : "sessionSignatureKey", "value" : "" } ], "url" : "https://mock-api.appsmith.com", "headers" : [ { "key" : "k11", "value" : "v11" } ] },

db.newAction.find({'unpublishedAction.name':'Api1234'}) { "_id" : ObjectId("6063233e7f7ba17ce75dbe06"), "applicationId" : "606322aa25d82b552ce8cded", "organizationId" : "5f9fc0550b682b2a63e424d3", "pluginType" : "API", "pluginId" : "5ca385dc81b37f0004b4db85", "unpublishedAction" : { "name" : "Api1234", "datasource" : { "_id" : ObjectId("606323c27f7ba17ce75dbe07"), "pluginId" : "5ca385dc81b37f0004b4db85", "updatedAt" : ISODate("2021-03-30T14:05:40.920Z"), "deleted" : false, "policies" : [ ] }, "pageId" : "606322aa25d82b552ce8cdef", "actionConfiguration" : { "timeoutInMillisecond" : 10000, "paginationType" : "NONE", "path" : "", "headers" : [ { "key" : "", "value" : "" }, { "key" : "", "value" : "" } ]

Nikhil-Nandagopal commented 3 years ago

@sumitsum there is no binding in your header as mentioned in the steps

sumitsum commented 3 years ago

@Nikhil-Nandagopal is the API header only supposed to be updated if the value comes from a binding and not other wise ? cc: @trishaanand

sumitsum commented 3 years ago

@Nikhil-Nandagopal I tried to used a binding in the header here. I am not able to see the headers getting updated. Please check if these steps look ok:

https://user-images.githubusercontent.com/1757421/113009823-eef32980-9195-11eb-93f5-12fafe0c2d36.mov

hetunandu commented 3 years ago

@Nikhil-Nandagopal, I don't think we should support this feature any more in this state. From my understanding, the use case is for custom login, and we should find a better way to enable this instead of building more on top of this unofficial feature. My suggestion is that we either create a new UX flow to allow for custom logins or if we want to enable bindings in datasources, we do it with the limited scope as today bindings are only at a page level while datasources are at an org level. AFAIK, the later is only possible in rest API today and it is not documented correctly as well.

Nikhil-Nandagopal commented 3 years ago

@hetunandu I believe this already works. The bug is that it only works after you refresh the page. So seems like a small fix isn't it?

hetunandu commented 3 years ago

@hetunandu I believe this already works. The bug is that it only works after you refresh the page. So seems like a small fix isn't it?

@Nikhil-Nandagopal It works, but it is a hack today due to how we have designed this system. The client is unaware that bindings can happen in a datasource form. We should start discussing a better way to do this instead of doing a hacky fix on top of a hack.

mohanarpit commented 3 years ago

Can I ask what's the use-case for supporting bindings in the datasource? A datasource is shared with all apps in an organization. Having bindings in the datasource can lead to subtle bugs/issues where users end up binding values that aren't available in other apps. That's why I'm asking what the use-case is here.

Nikhil-Nandagopal commented 3 years ago

@mohanarpit it's for sending the access token as Hetu mentioned.

mohanarpit commented 3 years ago

@hetunandu Is it possible for us to limit what we bind based on different scenarios? For example, in a datasource, a user can only bind {{env.key}} or {{appsmith.store.key}}?

hetunandu commented 3 years ago

Yes, we can limit the scope for datasources. We don't have any scope at the org level. appsmith.store is app-level. So including this datasource in another app will still confuse.

somangshu commented 3 years ago

Another user probably faced the same issue, Here are the details

  1. setup a rest datasource which returns a non-200 status code, like 401 or 404
  2. put some key/values in the headers RUN
  3. remove headers RUN look in the error message and you see that the headers are still transmitted it is simply not possible to remove them

The headers from the previous call are still present here until the page is refreshed. unknown (3)

@hetunandu @mohanarpit Is this a related issue?