Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow binding query parameters to environment variables #2777

Merged
merged 38 commits into from
Nov 7, 2023

Conversation

JerryWu1234
Copy link
Contributor

@JerryWu1234 JerryWu1234 commented Oct 10, 2023

#2616

  • I've read and followed the contributing guide on how to create great pull requests.
  • I've updated the relevant documentation for any new or updated feature.
  • I've linked relevant GitHub issue with "Closes #".
  • I've added a visual demonstration in the form of a screenshot or video.

@JerryWu1234
Copy link
Contributor Author

image

@JerryWu1234
Copy link
Contributor Author

image

@JerryWu1234
Copy link
Contributor Author

image

@Janpot
Copy link
Member

Janpot commented Oct 10, 2023

Ok, I think we can bring env var values to the editor frontend.

@apedroferreira
Copy link
Member

apedroferreira commented Oct 10, 2023

Agree, it should be fine to show the env values in the preview panel, it might even be better in some ways as it shows more useful information to the user.
But the queries that are not called in preview mode are still missing values for the parameters that are associated with environment variables, so we still need to make that work...

@JerryWu1234
Copy link
Contributor Author

Agree, it should be fine to show the env values in the preview panel, it might even be better in some ways as it shows more useful information to the user. But the queries that are not called in preview mode are still missing values for the parameters that are associated with environment variables, so we still need to make that work...

image
image

part of query

@JerryWu1234
Copy link
Contributor Author

I have a suggestion.
I saw that the backend will handle the env variable.
but now could we handle all the transformation in the editor front-end.
image

@apedroferreira @Janpot

@apedroferreira
Copy link
Member

apedroferreira commented Oct 11, 2023

I have a suggestion. I saw that the backend will handle the env variable. but now could we handle all the transformation in the editor front-end. image

@apedroferreira @Janpot

We want to be able to assign and use environment variables as Parameters - not really in Url query, as those bindings are already parsed in the server-side.
I think we can try passing the environment variable parameters to execBase as a new argument, maybe we can call it envParams?: : [string, BindableAttrValue<string>][] = []. But this will require refactoring in other places to make sure we always pass the environment bindings until they reach execBase.

OR (different idea) we can try making a new envParams that works the same as searchParams, but we still have to show those in the Parameters section.

@JerryWu1234
Copy link
Contributor Author

We want to be able to assign and use environment variables as Parameters - not really in Url query,
But the queries that are not called in preview mode are still missing values for the parameters that are associated with environment variables, so we still need to make that work...

I got it , and now it's going on.

image
image


I think we can try passing the environment variable parameters to execBase as a new argument, maybe we can call it envParams?: : [string, BindableAttrValue][] = []. But this will require refactoring in other places to make sure we always pass the environment bindings until they reach execBase.
OR (different idea) we can try making a new envParams that works the same as searchParams, but we still have to show those in the Parameters section.

there i'm not totally understand.
Because All of values we got it in the editor front-end.
we are parsed all value and pass to the editor back-end.

such as

we have a environment variable

SECRET_BAR="Some bar secret"

image

I create parameter and headers, finally I click the preview button, and send a require.

image

{
  "name": "dataSourceFetchPrivate",
  "params": [
    "rest",
    null,
    {
      "kind": "debugExec",
      "query": {
        "method": "GET",
        "headers": [
          [
            "var",
            {
              "$$env": "SECRET_BAR"
            }
          ]
        ],
        "browser": false,
        "searchParams": [
          [
            "aa",
            {
              "$$jsExpression": "`?code=${parameters.sd}`\n"
            }
          ]
        ]
      },
      "params": {
        "sd": "Some bar secret"
      }
    }
  ]
}

because we already have all of the environment variable.
We can directly parse this variable.
such as

"headers": [
            [
              "var",
              {
                "$$env": "SECRET_BAR"
              }
          ]

to  

"headers":{
  "var": "Some bar secret"
}

and 

"searchParams": [
          [
            "aa",
            {
              "$$jsExpression": "`?code=${parameters.sd}`\n"
            }
          ]
        ]
to 

"searchParams": {
  "aa": "?code=`Some bar secret"
}

final version

{
  "name": "dataSourceFetchPrivate",
  "params": [
    "rest",
    null,
    {
      "kind": "debugExec",
      "query": {
        "method": "GET",
        "headers": {
          "var": "Some bar secret"
        },
        "browser": false,
        "searchParams": {
          "aa": "?code=Some bar secre"
        }
      },
      "params": {
        "sd": "Some bar secret"
      }
    }
  ]
}

is there any problem in this way?
welcome point out problem

@Janpot
Copy link
Member

Janpot commented Oct 12, 2023

We have to make a distinction between the Toolpad editor and the Toolpad application. The latter is what gets served when running toolpad start. It's acceptable to have the environment variables available in the editor. Under no circumstance is it acceptable to have environment variables in the end user application. Not hidden, not ever. For the end user applications environment variables must always be resolved serverside.

Within the "private query" we can add a method to retrieve the environment variables of the process and variable names that exist in .env file. Those can be different. The former is used to resolve $$env bindings, the latter is used for autocomplete. It's only for preview purposes. To run the actual query, the env variables need to be calculated serverside.

@JerryWu1234
Copy link
Contributor Author

image

@JerryWu1234
Copy link
Contributor Author

@apedroferreira I done

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest changes are great, thank you!
We just still need to use the environment variables names in packages/toolpad-app/src/toolpad/AppEditor/BindingEditor.tsx, instead of the values.

We still also need to solve the main remaining problem: all these changes work for the "Preview" mode in the editor, but if you try it, you will see that they do not work in a live application. It only works when you click the "Preview" button. In all other cases you will see that the values are still undefined.
So we also need a way for the running application to be able to read and resolve environment bindings from the parameters - and we can't pass the env values there directly like we do in preview, because they can include secrets. We can only pass the bindings (of type [string, BindableAttrValue<string>][]), which then need to be resolved securely in the back-end.
I have already written some suggestions to try to solve this part of the problem before, in previous comments. But if you feel like that part is too much I can try to add a solution on top of your PR!

@JerryWu1234
Copy link
Contributor Author

JerryWu1234 commented Oct 17, 2023

I think definitely we weren't on the same page about preview.
two Preview buttons in the editor. LoL.

image

image

if I were right, you would want to say this preview

@JerryWu1234
Copy link
Contributor Author

But if you feel like that part is too much I can try to add a solution on top of your PR!

i will do it

JerryWu1234 and others added 2 commits October 26, 2023 16:32
Signed-off-by: Jerry_wu <409187100@qq.com>
@JerryWu1234
Copy link
Contributor Author

We can add tests later separately if you prefer, but in the end it would also be nice to have an integration test where we have an environment variable assigned to a parameter, and then we use it in the query headers by binding to the expression: Bearer ${params.envVarName}

I will add some tests if review is done

Signed-off-by: Jerry_wu <409187100@qq.com>
@JerryWu1234
Copy link
Contributor Author

Awesome, this approach seems to work!! Thank you so much! Just added a few more suggestions, but we're almost there!

Also 2 more things:

  1. Could we make the environment bindings not appear in the binding panel and be undefined in the client-side?
Screenshot 2023-10-24 at 23 29 46 2. We can add tests later separately if you prefer, but in the end it would also be nice to have an integration test where we have an environment variable assigned to a parameter, and then we use it in the query headers by binding to the expression: `Bearer ${params.envVarName}`

image

@JerryWu1234
Copy link
Contributor Author

@apedroferreira I done

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Some more comments:

@JerryWu1234
Copy link
Contributor Author

@apedroferreira please help check

@apedroferreira
Copy link
Member

Hi @JerryWu1234 , it's almost there but we still need to solve these cases, after that we can probably merge:

  1. When using bindings on the page, not only do I want the environment variable parameters to be undefined, but they should not even appear. They should be hidden.
Screen.Recording.2023-11-02.at.14.16.53.mov
  1. But in the query editor, I want to be able to see the real value, just like you had it before. Right now it splits the string into an array so the logic there is not working correctly:
Screen.Recording.2023-11-02.at.14.17.17.mov

@JerryWu1234
Copy link
Contributor Author

Hi @JerryWu1234 , it's almost there but we still need to solve these cases, after that we can probably merge:

  1. When using bindings on the page, not only do I want the environment variable parameters to be undefined, but they should not even appear. They should be hidden.

Screen.Recording.2023-11-02.at.14.16.53.mov
2. But in the query editor, I want to be able to see the real value, just like you had it before. Right now it splits the string into an array so the logic there is not working correctly:

Screen.Recording.2023-11-02.at.14.17.17.mov

image
image

@JerryWu1234
Copy link
Contributor Author

image

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your work so far @JerryWu1234 , I know it has been a long process. :)
You can just add this suggested change and then if there's nothing else you would like to add I think we should be good!

@apedroferreira apedroferreira changed the title fix by second way(a new feature that let user add env variable. ) Allow binding query parameters to environment variables Nov 7, 2023
@apedroferreira apedroferreira added the feature: Queries Making new API requests label Nov 7, 2023
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @JerryWu1234 , this was a really important feature!
We might add some refinements later but this solution will work great for now!

@apedroferreira apedroferreira merged commit 719bbab into mui:master Nov 7, 2023
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Queries Making new API requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants