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

Request.params do not allow duplicate parameters #247

Open
mildred opened this issue Apr 21, 2020 · 6 comments · May be fixed by #323
Open

Request.params do not allow duplicate parameters #247

mildred opened this issue Apr 21, 2020 · 6 comments · May be fixed by #323
Labels

Comments

@mildred
Copy link

mildred commented Apr 21, 2020

result[key] = val

Sometimes, in query strings, you need to pass lists. This is generally done by duplicating the same key over like: name=a&name=b. juster folds these into a single parameter in the above code. Instead of using []= to add new parameters, add allows key duplicates.

@sdmcallister
Copy link

What are people doing for this now?

@dom96
Copy link
Owner

dom96 commented Jun 30, 2021

would be nice to fix this

@dom96 dom96 added the Feature label Jun 30, 2021
@torbencarstens
Copy link

Hey @dom96, I'd be interested to implement this since I want to use it myself (to use <select multiple> with GET).

Before creating a PR I just wanted to get some input from you regarding the implementation.

tables.add

I'd avoid this since it has been deprecated since nim 1.4.

joining values on a character

Since 0.6.0 added the auto decoding of query values simply joining the different values on some reserved character (e.g. #) is not really viable anymore.

-> This would fail as soon as the user has a percent-endoding for the "join character" in their query value (e.g. a=1&a=b%23c would be returned in the following way 1#b#c).

changing the return type

This would completely break all existing programs relying on params.

custom ParamValue type

Changing the return type to Table[str, ParamValue] where ParamValue would be something like (note that this is basically a copy of the JsonNode implementation)

ParamValue implementation
type
  ParamValueKind* = enum
    ParamString
    ParamList

  ParamValueObj* = object
    case kind*: ParamValueKind
    of ParamString:
      str*: string
    of ParamList:
      list*: seq[string]

  ParamValue* = ref ParamValueObj

which could be constructed like this

let stringParam = ParamValue(kind: ParamString, str: "2")
let listParam = ParamValue(kind: ParamList, list: @["1", "2"])

Additionally some helper functions to retrieve a specific type could be added as well (e.g. getListParam).

Table[str, seq[str]]

Another option would be to simply use Table[str, seq[str]], since query parameters occuring multiple times is the odd case this wouldn't be a nice default.

Additional function

Adding a new function (e.g. paramsListValues) might be the option with the least user impact.

Implemented by either option in changing the return type.

Downside of this option is that it would basically mean a 1:1 implementation of params with the exception of the result type (and it's creation).

[...]


One general thing about the params function, is there any special reason for this

jester/jester/request.nim

Lines 112 to 117 in 053d62c

let contentType = req.headers.getOrDefault("Content-Type")
if contentType.startswith("application/x-www-form-urlencoded"):
try:
parseUrlQuery(req.body, result)
except:
logging.warn("Could not parse URL query.")
part?

parseUrlQuery is deprecated and points towards cgi/decodeData which you're already using in params.

The main difference seems to be that key is also url decoded in parseUrlQuery.

@ThomasTJdev
Copy link
Contributor

@torbencarstens,
How about a helper proc which parses req.url? That would require the user to call it manually to avoid the overhead of parsing twice.

@torbencarstens
Copy link

torbencarstens commented Sep 17, 2023

Would you mind elaborating a bit on this, I guess you agree with implementing the Additional function part of my comment above?

This would implicitely avoid parsing twice unless the user calls both params and {newFunction} which would be ... interesting.

I wouldn't use the req.url though since the query proc is already there and the return value can be used for the cgi.decodeData proc argument. (although we could use cgi.decodeQuery directly as well since decodeData(string) is just decodeQuery in disguise).

Users calling both query and params/{newFunction} is probably rather rare as well (and rather inexpensive at that if it happens).

This solution would basically look like this (I chose Table[string, seq[string]] here for breviety since the ParamValue implementation would be a bit longer)

proc paramsValuesAsList*(req: Request): Table[string, seq[string]] =
  ## Parameters from the pattern and the query string.
  ##
  ## In contrast to `params` this supports query keys occuring multiple times (e.g. `?a=1&a=2`).
  # deliberately ignoring `patternParams` here for brevity since it needs a mapping from `string` -> `seq[string]`
  result = initTable[string, seq[string]]()

  let query = query(req)

  try:
    for key, val in cgi.decodeQuery(query):
      let decodedValue = decodeUrl(val)

      # this assumes that the key url decoding is irrelevant (only implemented for `x-www-form-urlencoded` queries as of now)
      if result.hasKey(key):
        result[key].add decodedValue
      else:
        result[key] = @[decodedValue]
  except CgiError:
    logging.warn("Incorrect query. Got: $1" % [query])

  result

(this assumes that the parseUrlQuery function is currently unnecessary)


As a side note regarding the What are people doing for this now?, if you want to work around this on the "client" side you can basically take the function from above, put it in your own program and pass the request variable to it.

@ThomasTJdev
Copy link
Contributor

[..]

My suggestion was for a way to do it without changing jester-code, but just hooking in on available fields = the .url

For your suggested solutions, I'm in favor of the Additional function. Otherwise you should change it globally to the Table[string, seq[string]] and with the param template @ just return the first value as a string, so it resembles the current approach.

torbencarstens added a commit to torbencarstens/jester that referenced this issue Sep 18, 2023
…Seq`

In contrast to `params` this returns a `Table[string, seq[string]]`.

Closes dom96#247
torbencarstens added a commit to torbencarstens/jester that referenced this issue Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants