-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Bug: Pre-request script is executed before the request is prepared #2249
Comments
The pre-request script phase is now executed after configureRequest. That means the 'req' object in pre-request script has access to more data, including evaluated variables, set authorization headers, etc. fixes usebruno#2249
Hey @pietrygamat We have a fix in place for this, which is planned to be included in the upcoming release. |
Wow :(
For the context - because bruno does not support raw file (and I want some extra control over file selection) I've implemented this script to send raw binary content in post request. few versions ago it was fully functional script, now it's not working at all. Not sure what exactly was broken but errors or console logs from pre-req are not delivered anywhere and this file is not being sent to api eiter Bruno version: 1.17.0 |
@lohxt1 do you have an update on this one, or a public branch where this can be peeked at?
|
@pietrygamat check if it's fixed in 1.18 which is just dropped in release channel. It's fixed my scripts with file uploads for sure. |
The current behavior in v1.18.0, is that all the interpolation happens at the end of pre-request script execution. Issue with configuring request before pre-request script : I was wondering if introducing a new bru function called Scenario:
Let me knwo what you think of this |
Could you verify whether your issue was resolved with version 1.18.0? |
This is a good point. I have been thinking under different assumptions: the script would be able to replace variables on those variables that have not been already replaced (no value provided in env file, etc), but I can see how this is not always the case. In that context proposed solution seems like a reasonable nice-to-have. That, however, does not solve my core issue here: the So having a script execute on a fully prepared request would allow accessing anything that the general configuration sets for the user (including access_token) and would also theoretically prevent edge cases, where something set in the pre-request script is overwritten by default bruno behavior. But it is a sort of a chicken-egg problem, because one may claim they don't want to read anything set in configuration, but rather feed If you vote to stay with current behavior, this may be closed. |
@lohxt1 yes it does, thank you. I tested it when I saw 1.18 was pushed to brew.sh Re With our samples above - it's clear that there are some scenarios when you want to change vars before interpolation (like inferring a full url or adjusting the path like in @pietrygamat scenario) and some cases where we want to have a configured request and re-write parts of it (my file uploads workaround). Taking a step back from solution here I think end result should satisfy both behaviours:
As an idea - sequence of steps can be something like this:
So it's the sequence that is currently in place with an ability to force skip automatic interpolation, but not proxy\cookies settings. Thoughts ? |
I see that you have another PR (#2077) which supersedes #2061? Please correct me if I'm wrong, but going through the PR, I see why you'd prefer the request configuration to happen before the pre-request script. When the access_token is obtained through either the authorization process or from the oauth2Store (cached), it gets set to the request object in the configuration step (which takes place after the pre-request execution) Because of this, req.credentials is undefined in the pre-request script but has the correct values in the post-response script. I'm assuming that you implemented this solution due to the absence of dedicated variables for storing the access_token value. If we had dedicated global variables to store these special tokens, we could use them instead of the oauth2Store as in the current implementation. This would mean that you'd no more need request.credentials, and you'd be okay with the current steps ? i.e pre-request script execution I remember having a discussion around dedicated variables in one of the older threads. A new discussion has been initiated by Anoop regarding the variables in Bruno: #2332 There is a plan to add a new set of variables. Feel free to share your thoughts in this discussion. |
@centur one scenario that I can think of where Before: After: In the screenshot the function is called as
currently you can directly access the variable values, modify them and assign them to parts of request object (req.setHeader, req.setBody) in the pre-request script, which otherwise would have been interpolated at the end of pre-request script execution if untouched. For example: In headers:
In pre-request script:
i'm not entirely sure at the moment whether we'd be needing |
That's right. It may be easier to manage them separately - to sort backend logic in #2061 and required UI changes in #2077 (which I will rebase as required) - or they may be considered a monolitic whole, in which case we can close the older one.
It's not just that. I like the idea of reusing the session cache also for tokens, because they are stored together and cleaned together easily with the same button, and also survive the application restart without being committed to bru files. But that's a discussion better moved to #2061 or #2077 when you get to it.
Depends on the steps - the variables still need to be set somewhere. If we manually click the get access token button, the token request is executed outside of the flow, the variables are set and are available. If however we want to rely on bruno to obtain the token on request run (for example when running from cli), the variables are not present until the configure stage is reached. It's the same case for any other value set in configure stage. Pre-request script is unable to:
Having power to do so seems nice, no? |
oh i'm totally onboard with the idea of using the session cache, this was the same thought process behind using oauth2Store to manage the auth window sessions :)
I meant to say And I was mainly talking about how the cache values are set to
this can be done overwrite_header.movAn option to append a certificate per request can be achieved by adding a new additional input field that can be set to a variable like '{{caFilePath}}' along with the existing filepicker and can be modified in the pre-request script, and the option to use variables for proxy input fields is already present. I also think that the way you are fetching the token as part of the configure step in #2077 makes more sense. (keeping the need for having dedicated variables out of the current discussion) In the request-level pre-request script, I would then access 'req.credentials' var and modify the request headers/body/url as per my needs to pass the token value. (this type of usage is rare?) the pros of having pre-request script execution before the configure-request step outweigh the cons i think |
The hints look like they should work. Let's close this one then. |
I have checked the following:
Describe the bug
When executing the request, bruno goes through several stages of configuration, among them: execution o pre-request script, post-request script, and run of 'configureRequest` method. The current execution order is:
I wonder if it would make more sense to execute the steps in this order:
This way any logic built into bruno may be overwritten by user's script and on top of that, user's script execute on request that is closer to what is actually being sent (e.g. with variables resolved, headers set).
For example, the below bru file logs this to console:
while I would expect the output Before and After to be the same.
.bru file to reproduce the bug
meta {
name: example
type: http
}
post {
url: {{api_url}}
body: text
auth: basic
}
headers {
Content-Type: application/json
}
auth:basic {
username: user
password: password
}
script:pre-request {
console.log('Before');
console.log(req.url);
console.log(JSON.stringify(req.getHeaders()));
}
script:post-response {
console.log('After');
console.log(req.url);
console.log(JSON.stringify(req.getHeaders()));
}
Screenshots/Live demo link
N/A
The text was updated successfully, but these errors were encountered: