-
Notifications
You must be signed in to change notification settings - Fork 392
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
Addition of period into variable declaration #3582
Addition of period into variable declaration #3582
Conversation
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpRequestParser.cs
Outdated
Show resolved
Hide resolved
…estParser.cs Co-authored-by: Jon Sequeira <jonsequeira@gmail.com>
@@ -194,7 +194,7 @@ private HttpVariableDeclarationNode ParseVariableDeclaration() | |||
|
|||
while (MoreTokens()) | |||
{ | |||
if (CurrentToken is { Text: "@" } or { Kind: HttpTokenKind.Word } or { Text: "_" }) | |||
if (CurrentToken is { Kind: HttpTokenKind.Word } or { Text: "@" or "_" or "." }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: Can this cause conflicts for the reference syntax for named request variables down the line?
For example, if we have a first-class variable named host.response.body
and another named request variable named host
, then it is ambiguous whether host.response.body
refers to the variable or to the body of the remembered response for the named request.
This is probably ok so long as we have proper conflict resolution rules in the expression binding (for example, we could always bind to first-class variables before checking for matching named request variables and / or we could look at the expression to see if JSONPath / XPath is involved and is yes prefer the named request variable)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely think it could be a discrepancy in the future, but I personally believe that we should prioritize named requests for expression binding and if there is no named request available, then we should look for first-class variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably ok so long as we have proper conflict resolution rules in the expression binding (for example, we could always bind to first-class variables before checking for matching named request variables and / or we could look at the expression to see if JSONPath / XPath is involved and is yes prefer the named request variable)...
The potential to have two variables where one silently shadows another could lead to some really hard-to-diagnose bugs for users. I'm not convinced that the ability to have a .
in a variable name adds value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree - the more ideal outcome would be to say that .
is not valid in names. Most other languages don't allow this either.
Do we know if the variable names with .
were supported in previous versions of the http editor by accident or was this by design? Do we know how broad the break is - is this just a couple of users here and there who happened to use .
or was there some bigger reason to do so (e.g., advertised in examples in docs / existing convention for env files etc.)?
Also tagging @phenning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the period in names was supported erroneously before so users are noticing it as a breaking change with the new parser. Several users reported the issue or upvoted it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we already have this "variable shadowing" in other areas where the explicitly defined variable wins.
eg. if a customer has an httpenv.json with:
"dev": {
"queryVar": "value"
}
and the http file
@queryVar = "differentValue"
GET https://somehost.org/?query={{queryVar}}
The one in the http file will be used, intentionally.
It is actually very easy for a customer to get into this scenario, just create a Web API or Aspire Starter project with dots in the name and the generated http file by the template will have a dot in it, as we simply use the SafeProjectName in the substitution, which will include dots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add, the reason for the explicitly named variable "winning" over the http-environment declaration of the file is because it is the most local declaration. I think the same thing would apply when the named requests are implemented and suggest that a declaration like
@login.response.headers.x-AuthToken = dsgyds87gss
Would be used in place of the value from
# @name login.
Simply because if the variable is being declared, we should take the customer at face value and assume that is what they wanted since the explicitly declared it.
The real world scenario here for this would be in cases where I was using the result in a named request in one API, but wanted to quickly test another specific value without rewriting my query. This would also be consistent with the priority between httpenv.json and locally declared variables.
This PR would update the http parser to support periods in variable declarations. This is a slight feature that has been requested by some users