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.

Sign up for GitHub

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

Jump to bottom

Add support for duplicate keys in parameters by adding paramValuesAsSeq #323

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Add support for duplicate keys in parameters by adding paramValuesAsSeq #323

wants to merge 20 commits into from

Conversation

torbencarstens
Copy link

@torbencarstens torbencarstens commented Sep 18, 2023 •
edited
Loading

Closes #247

I've implemented an example solution (new function) so that we can discuss further based on this.

I've opened this as a PR since the actual implementation discussion is not really relevant for the issue itself.

I'm sorry to say that I'm still not entirely sure how the following would work:

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

I'm still fairly new to nim (<1000 lines) so forgive me if I'm missing something obvious here, the only options I see to implement this are

  • experimental features (e.g. dotOperators)
  • adding a url property (new type?) to Request which supports this feature (this modifies code even further though and would be inconsistent with the current implementation)
  • probably templates/macros similar to promoting a proc to a different implementation when explicitely called by the user (or the library)
  • implementing this for both httpbeast and the standard library and expose this via jester (also needs code changes since the NativeRequest isn't exposed to the user) (also unlikely to be accepted)

That being said, this should probably be a separate function in any case which can be used by any of the other options if another shortcut/accessor is implemented for this.

I'm not entirely happy with the name (feel free to suggest others) but I hope it's at least clear enough on what it does.

I've removed the explicit call to decodeUrl(value) (in both params and paramValuesAsSeq) since decodeData (decodeQuery internally) does the decoding for us already.

parseUrlQuery

I've also updated the params function to not use parseUrlQuery anymore since this has been marked as deprecated in any case.

With this change the 2 function operate essentially the same exact way (excluding the return type).

I haven't removed it since it's exported and might be used by users.

Tests

I've also implemented some tests for both params and paramValuesAsSeq (exactly the same tests with different results).

I tested this against the old version of params and the new version.

CI

os

Currently the CI:

  • fails on windows
    • I don't really have access to a windows machine right now and have no idea what the problem could be
  • aborts on mac due to the runner coming up late
  • succeeds on linux
versions

CI succeeds for nim 1.4.8 and 1.6.14, for nim 2 the CI throws a SIGSEGV: Illegal storage access. (Attempt to read from nil?).

I've observed this behaviour with jester before and either running nim with --mm:refc or importing std/segfaults fixed this (which is very frustrating).

I've only tested running with --mm:refc which didn't fix it in the CI, I didn't want to import std/segfaults in all tests only for the CI to work since this is obviously a bug somewhere.

I haven't looked into this any closer though, everything (tests + jester itself) run fine for me with nim 2.0 (locally and in docker).

Comments on things I've updated which aren't directly related to the issue. I hope including this here is fine with you, otherwise I don't mind pulling these changes out and opening a separate PR with them.

Exclude testing tests/nim-in-action-code for nim >= 2.0

nim-in-action-code isn't trying to be compatible with nim 2 (as far as I can tell).

The compilation currently fails because db_sqlite isn't available anymore (needs a nimble install db_connector and a change of import, i.e. -> db_connector/db_sqlite

Update the CI

Basically this #319, I didn't want a broken CI when submitting a PR.

Update nimble test

Up to now the user had to execute some steps themselves (e.g. refresh/install), nimble test should now work without any further input from the user after cloning the repository.

.gitignore

I've developed with intellij so I've added the common ignores for this IDE

has been reverted (see #323 (comment))

@torbencarstens
Add common intellij paths to .gitignore
a4111d9
@torbencarstens
Use 1.6.14 in CI instead of pre-1.6.0 git sha
ad46aa4
@torbencarstens
Add 2.0.0 to test matrix
08bec4a
@torbencarstens
Only test nim-in-action-code/Chapter7 for nim versions < 2.0
2fe8aa7
@torbencarstens
Bump actions/checkout and iffy/install-nim
c631652
@torbencarstens
Run 'git submodule update --init' for 'nimble test' to ensure that al…
f05bd2b 
…l test requirements are present

Only do this when nim version < 2.0
@torbencarstens
Add 'nimble refresh' and 'nimble install' for 'nimble test' task befo…
88b2ace 
…re running the tests
@torbencarstens
Remove trailing whitespace
5c9e5d3
@torbencarstens
Add support for duplicate keys in parameters by adding `paramValuesAs…
fef495a 
…Seq`

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

Closes dom96#247
@torbencarstens
Adapt params to use cgi.decodeData instead of parseUrlQuery
b8374b4 
This is also the same implementation used in `paramValuesAsSeq` (except
for the return type), this way we ensure the same behaviour for the
parsing/decoding steps.
@torbencarstens
Add tests for dom96#247
f916b25
@torbencarstens
Fix argument name to iffy/install-nim
d330fad
@torbencarstens
Auto accept nimble for refresh/install
1cce8cc
@torbencarstens
Run debug actions in CI
944dc39
@torbencarstens
Only debug 2.0.0
149d5bf
@torbencarstens
Remove debug actions
88e4a29
@torbencarstens
Run nim2 with '--mm:refc'
cda4d39
@torbencarstens
Remove nim2 from CI
85b2692 
Currently the server throws a 'SIGSEGV: Illegal storage access. (Attempt
to read from nil?)'
@torbencarstens
Remove debug when
84035bf
@torbencarstens torbencarstens marked this pull request as ready for review September 18, 2023 22:23
@ThomasTJdev
Copy link
Contributor

Access url field

I'm sorry to say that I'm still not entirely sure how #247 (comment):

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

My mistake - I was sure, that there were a .url field which was public available, so you could roll you own procs on top. That would also have allowed users to implement their own query-collector, whether it is a Table[string, seq[string]] or something else.

Failing CI on nim v2

This is when httpbeast is used. The httpbeast#head includes a fix, but it must be published with a new tag (dom96/httpbeast#91).

@torbencarstens
Copy link
Author

CI

nim v2

Alright, thanks for the info. I guess the addition of this CI version can be done at a later point then.

Windows

Regarding the CI on windows, the exact error message is (nim: 1.6.14)

C:\Users\{user}\.nimble\pkgs\asynctools-#0e6bdc3ed5bae8c7cc9\asynctools\asyncpipe.nim(260, 16) Error: undeclared identifier: 'PCustomOverlapped'

I found the following issue in asynctools: cheatfate/asynctools#31 which seems to explain this.

How do you want to proceed with this?

Aside from these 2 issues, is there anything left which has to be addressed (additional reviews/changes/documentation/examples/...)?

I guess at least the CI workflow still has to be approved and we need to test this on the mac pipeline (needs a decision on how the current windows bug should be handled).

@ThomasTJdev
Copy link
Contributor

The .gitignore has two new files, which might be due to your editor preference - they can be removed.

For the asynctools I dont know - I'm not running windows... sorry.

I like the PR and your additional tests 👍. But, I'm just a contributor to the repo, so it must be @dom96 who can give you the directions for the next step.

@torbencarstens
Copy link
Author

Alright, thanks for your help. I've reverted the .gitignore commit.

I'll wait for some input from dom then.

@torbencarstens
Copy link
Author

torbencarstens commented Sep 19, 2023 •
edited
Loading

Note about the Windows CI failures, I've fixed the asynctools issues locally but the tests still fail (explicitly import asyncpty and use asyncpty.PCustomOverlapped directly in asyncipc/asyncpipe).

I ran these on the current master to ensure that none of my changes has any impact on this. (I've ran mine as well and they're all [OK]).

The following test suites fail (I haven't looked into why this is):

  • "static" / "index.html"
  • "static" / "test_file.txt"
  • "before/after" / "before - 404"

Note that the kill ${PID} message is a bit misleading due to the finally block error being directly underneath, the actual kill output comes from here https://github.com/dom96/httpbeast/blob/17e322ba68b264e9f4ec755e6946c64242d289ad/tests/tester.nim#L32 and is thrown in the allTest(useStdLib=true) testsuite when startServer is called.

I guess the impact from the Windows CI failing should be ignored in this case since it fails due to conditions which are already on master, this should probably be fixed separately.

logs for nim c --run tests/tester.nim (nim 1.6.14) 
Hint: used config file 'C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\config\nim.cfg' [Conf]
Hint: used config file 'C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\config\config.nims' [Conf]
Hint: used config file 'C:\Users\{user}\projects\jester\tests\config.nims' [Conf]
Hint: gc: refc; opt: none (DEBUG BUILD, `-d:release` generates faster code)
19512 lines; 0.028s; 16.57MiB peakmem; proj: C:\Users\{user}\projects\jester\tests\tester.nim; out: C:\Users\{user}\projects\jester\tests\tester.exe [SuccessX]
Hint: C:\Users\{user}\projects\jester\tests\tester.exe  [Exec]
  Verifying dependencies for jester@0.6.0
  Compiling tests\alltest (from package jester) using c backend
C:\Users\{user}\projects\jester\jester\private\utils.nim(23, 18) Warning: inherit from a more precise exception type like ValueError, IOError or OSError. If these don't suit, inherit from CatchableError or Defect. [InheritFromException]
C:\Users\{user}\projects\jester\jester\private\utils.nim(41, 37) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester\private\utils.nim(103, 12) Warning: Deprecated since v1.4; it was more confusing than useful, use `[]=`; add is deprecated [Deprecated]
C:\Users\{user}\projects\jester\jester\request.nim(115, 7) Warning: use stdlib cgi/decodeData; parseUrlQuery is deprecated [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(183, 23) template/generic instantiation of `async` from here
C:\Users\{user}\projects\jester\jester.nim(186, 8) Warning: use fileExists; existsFile is deprecated [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(320, 27) template/generic instantiation of `async` from here
C:\Users\{user}\projects\jester\jester.nim(333, 8) Warning: use dirExists; existsDir is deprecated [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(547, 72) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(568, 34) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(577, 75) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(589, 36) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(595, 58) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(606, 33) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(615, 47) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(632, 19) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(639, 34) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(646, 34) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(658, 19) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(663, 33) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(666, 34) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(669, 50) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(672, 38) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\jester.nim(685, 39) Warning: `typed` will change its meaning in future versions of Nim. `void` or no return type declaration at all has the same meaning as the current meaning of `typed` as return type declaration. [Deprecated]
C:\Users\{user}\projects\jester\tests\alltest.nim(9, 19) Warning: inherit from a more precise exception type like ValueError, IOError or OSError. If these don't suit, inherit from CatchableError or Defect. [InheritFromException]
C:\Users\{user}\projects\jester\tests\alltest.nim(10, 23) Warning: inherit from a more precise exception type like ValueError, IOError or OSError. If these don't suit, inherit from CatchableError or Defect. [InheritFromException]
C:\Users\{user}\projects\jester\tests\alltest.nim(29, 1) template/generic instantiation of `routes` from here
C:\Users\{user}\projects\jester\jester.nim(1352, 35) template/generic instantiation of `async` from here
C:\Users\{user}\projects\jester\tests\alltest.nim(44, 10) Warning: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
C:\Users\{user}\projects\jester\tests\alltest.nim(29, 1) template/generic instantiation of `routes` from here
C:\Users\{user}\projects\jester\jester.nim(1352, 35) template/generic instantiation of `async` from here
C:\Users\{user}\projects\jester\tests\alltest.nim(143, 10) Warning: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
C:\Users\{user}\projects\jester\tests\alltest.nim(6, 8) Warning: imported and not used: 'alltest_router2' [UnusedImport]
C:\Users\{user}\projects\jester\tests\alltest.nim(3, 53) Warning: imported and not used: 'asyncnet' [UnusedImport]
   Success: Execution finished
Getting http://localhost:5454 - attempt 0
Process: INFO Jester is making jokes at http://127.0.0.1:5454/foo
Process: ERROR C:\Users\{user}\projects\jester\tests\alltest.nim(29) alltest
Process: C:\Users\{user}\projects\jester\jester.nim(545) serve
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncdispatch.nim(1956) runForever
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncdispatch.nim(1653) poll
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncdispatch.nim(419) runOnce
Server started!
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncdispatch.nim(234) processPendingCallbacks
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(28) serveNimAsyncContinue
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asynchttpserver.nim(425) serveIter
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(232) processClient
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(28) processClientNimAsyncContinue
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asynchttpserver.nim(362) processClientIter
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(232) processRequest
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(31) processRequestNimAsyncContinue
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asynchttpserver.nim(330) processRequestIter
Process: C:\Users\{user}\projects\jester\jester.nim(539) :anonymous
Process: C:\Users\{user}\projects\jester\jester.nim(399) handleRequest
Process: C:\Users\{user}\projects\jester\jester\request.nim(172) pathInfo
Process: C:\Users\{user}\projects\jester\jester\request.nim(166) stripAppName
Process: Expected script name at beginning of path. Got path: / script name: /foo
Process: DEBUG   502 Bad Gateway {"content-type": @["text/html;charset=utf-8"]}
Process: ERROR C:\Users\{user}\projects\jester\tests\alltest.nim(29) alltest
Process: C:\Users\{user}\projects\jester\jester.nim(545) serve
[OK] doesn't crash on missing script name
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncdispatch.nim(1956) runForever
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncdispatch.nim(1653) poll
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncdispatch.nim(419) runOnce
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncdispatch.nim(234) processPendingCallbacks
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(28) serveNimAsyncContinue
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asynchttpserver.nim(425) serveIter
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(232) processClient
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(28) processClientNimAsyncContinue
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asynchttpserver.nim(362) processClientIter
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(232) processRequest
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asyncmacro.nim(31) processRequestNimAsyncContinue
Process: C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\pure\asynchttpserver.nim(330) processRequestIter
Process: C:\Users\{user}\projects\jester\jester.nim(539) :anonymous
Process: C:\Users\{user}\projects\jester\jester.nim(399) handleRequest
Process: C:\Users\{user}\projects\jester\jester\request.nim(172) pathInfo
Process: C:\Users\{user}\projects\jester\jester\request.nim(166) stripAppName
Process: Expected script name at beginning of path. Got path: / script name: /foo
[OK] can access root
[OK] /nil
[OK] /halt
[OK] /halt-before
[OK] /guess
[OK] /redirect
[OK] /redirect-halt
[OK] /redirect-before
[OK] regex
[OK] resp
[OK] template
[OK] json
[OK] sendFile
[OK] can access query
[OK] can access querystring
[OK] issue 157
[OK] resp doesn't overwrite headers

[Suite] static
    C:\Users\{user}\projects\jester\tests\tester.nim(169, 27): Check failed: body.startsWith("This should be available at /root/.")
  [FAILED] index.html
    C:\Users\{user}\projects\jester\tests\tester.nim(173, 32): Check failed: (waitFor resp.body) == "Hello World!"
    (waitFor resp.body) was <html xmlns="http://www.w3.org/1999/xhtml"><head><title>400 Bad Request</title></head><body style="text-align: center;"><h1>400 Bad Request</h1><hr/><p>Jester 0.6.0</p></body></html>
  [FAILED] test_file.txt
  [OK] detects attempts to read parent dirs

[Suite] extends
  [OK] simple
  [OK] params
  [OK] separate module
  [OK] external regex
  [OK] regex path prefix escaped

[Suite] error
  [OK] exception
  [OK] HttpCode handling
  [OK] `pass` in error handler
    C:\Users\{user}\projects\jester\tests\tester.nim(220, 32): Check failed: (waitFor resp.body) == "404 not found!!!"
    (waitFor resp.body) was <html xmlns="http://www.w3.org/1999/xhtml"><head><title>400 Bad Request</title></head><body style="text-align: center;"><h1>400 Bad Request</h1><hr/><p>Jester 0.6.0</p></body></html>
  [FAILED] custom 404

[Suite] before/after
  [OK] before - halt
  [OK] before - unaffected
  [OK] before - global
    C:\Users\{user}\projects\jester\tests\tester.nim(237, 22): Check failed: resp.code == Http404
    resp.code was 400 Bad Request
    Http404 was 404 Not Found
  [FAILED] before - 404
  [OK] after - added
kill: 17728: No such process
kill: 17728: No such process
C:\Users\{user}\projects\jester\tests\tester.nim(296) tester
C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\system\assertions.nim(38) failedAssertImpl
C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\system\assertions.nim(28) raiseAssert
C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\system\fatal.nim(54) sysFatal
Error: unhandled exception: C:\Users\{user}\projects\jester\tests\tester.nim(296, 14) `execCmd("kill -15 " & $serverProcess.processID()) == QuitSuccess`  [AssertionDefect]
Error: execution of an external program failed: 'C:\Users\{user}\projects\jester\tests\tester.exe '

@ThomasTJdev ThomasTJdev mentioned this pull request Jan 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Request.params do not allow duplicate parameters
2 participants
@torbencarstens @ThomasTJdev