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

qjs fetch. #864

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

qjs fetch. #864

wants to merge 1 commit into from
+3,608 −6

Conversation

hongzhidao
Copy link
Contributor

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue using one of the supported keywords here in this description (not in the title of the PR).

Checklist

Before creating a PR, run through this checklist and mark each as complete:

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes

@hongzhidao
Copy link
Contributor Author

Hi @xeioex,
Here's a draft PR for QuickJS fetch support. And it passed js_fetch_object.t locally.
My next plans are:

  1. Refactor out prepared patches which are refactoring and fixing patches.
  2. Merge ngx_qjs_fetch.c into ngx_js_fetch.c and reuse code as much as possible.
  3. Support https.
  4. Test with njs-sandbox.

@hongzhidao hongzhidao closed this Mar 19, 2025
@hongzhidao hongzhidao reopened this Mar 19, 2025
@hongzhidao
Copy link
Contributor Author

Hi @xeioex,
Take a look at this.
https://github.com/nginx/njs/blob/master/nginx/ngx_js_shared_dict.c#L3042
It caused the assert error.

void JS_SetClassProto(JSContext *ctx, JSClassID class_id, JSValue obj)
{
    JSRuntime *rt = ctx->rt;
    assert(class_id < rt->class_count);
    set_value(ctx, &ctx->class_proto[class_id], obj);
}

Is it because there is no JS_NewClass() for NGX_QJS_CLASS_ID_SHARED_DICT_ERROR?

@xeioex
Copy link
Contributor

xeioex commented Mar 24, 2025 •
edited
Loading

Hi @hongzhidao,

It seems, with more classes you added you stepped upon rt->class_count limit. The class array is resized by 3/2. So, yes, we need JS_NewClass() for NGX_QJS_CLASS_ID_SHARED_DICT_ERROR.

@hongzhidao
Copy link
Contributor Author

hongzhidao commented Mar 24, 2025 •
edited
Loading

Hi @hongzhidao,

It seems, with more classes you added you stepped upon rt->class_count limit. The class array is resized by 3/2. So, yes, it seems, we need JS_NewClass() for NGX_QJS_CLASS_ID_SHARED_DICT_ERROR.

Agree, thanks.
I'll fix it, but I need to make this PR pass with js_fetch.t first.

[update] done: #873

@hongzhidao hongzhidao mentioned this pull request Mar 24, 2025
Merged
@hongzhidao hongzhidao force-pushed the qjs-fetch branch 4 times, most recently from 89600cd to 936da2c Compare March 25, 2025 06:49
@hongzhidao
Copy link
Contributor Author

Hi @xeioex,
Does the test pass when you run it?

njs/nginx/t/js_fetch.t

Line 427 in 8708663

like(http_get('/body?getter=json&loc=json&path=b.c'),

I tested it with njs, and it failed. I debug the response. Its output is:

HTTP/1.1 403 Forbidden
Server: nginx/1.27.5
Date: Tue, 25 Mar 2025 07:22:53 GMT
Content-Type: text/html
Content-Length: 153
Connection: close

<html>
<head><title>403 Forbidden</title></head>
<body>
<center><h1>403 Forbidden</h1></center>
<hr><center>nginx/1.27.5</center>
</body>
</html>

@xeioex
Copy link
Contributor

xeioex commented Mar 26, 2025

@hongzhidao

here is my js_fetch.t output

TEST_NGINX_VERBOSE=1 TEST_NGINX_LEAVE=1 TEST_NGINX_BINARY=/path/to/objs/nginx prove -I /path/to/nginx-tests/lib/ nginx/t/js_fetch.t -v 

nginx/t/js_fetch.t .. 
# >> GET /engine HTTP/1.0\x0a
# >> Host: localhost\x0a
# >> \x0a
# << HTTP/1.1 200 OK\x0d\x0a
# << Server: nginx/1.27.4\x0d\x0a
# << Date: Wed, 26 Mar 2025 02:04:37 GMT\x0d\x0a
# << Content-Type: text/plain\x0d\x0a
# << Content-Length: 3\x0d\x0a
# << Connection: close\x0d\x0a
# << \x0d\x0a
# << njs
1..39

...

# >> GET /body?getter=text&loc=loc HTTP/1.0\x0a
# >> Host: localhost\x0a
# >> \x0a
# << HTTP/1.1 200 OK\x0d\x0a
# << Server: nginx/1.27.4\x0d\x0a
# << Date: Wed, 26 Mar 2025 02:04:37 GMT\x0d\x0a
# << Content-Type: text/plain\x0d\x0a
# << Content-Length: 7\x0d\x0a
# << Connection: close\x0d\x0a
# << \x0d\x0a
# << "GET::"
ok 2 - fetch body text
# >> GET /body?getter=json&loc=json&path=b.c HTTP/1.0\x0a
# >> Host: localhost\x0a
# >> \x0a
# << HTTP/1.1 200 OK\x0d\x0a
# << Server: nginx/1.27.4\x0d\x0a
# << Date: Wed, 26 Mar 2025 02:04:37 GMT\x0d\x0a
# << Content-Type: text/plain\x0d\x0a
# << Content-Length: 7\x0d\x0a
# << Connection: close\x0d\x0a
# << \x0d\x0a
# << "FIELD"
ok 3 - fetch body json

...

ok
All tests successful.
Files=1, Tests=39,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.17 cusr  0.05 csys =  0.24 CPU)
Result: PASS

@hongzhidao
Copy link
Contributor Author

hongzhidao commented Mar 26, 2025 •
edited
Loading

I changed the location /json, and then it passed.

location /json { return 200 '{"a":[1,2], "b":{"c":"FIELD"}}'; }

It passed with non-root user.

@hongzhidao
Copy link
Contributor Author

hongzhidao commented Mar 26, 2025 •
edited
Loading

Hi @xeioex,
js_fetch.t and js_fetch_objects.t passed except this case.

like(http_get('/body?getter=json&loc=loc'), qr/501/s,
    'fetch body json invalid');

But I'm not sure if it's an issue in QuickJS.
bellard/quickjs#341
Created an issue to track it.
bellard/quickjs#395

My next step is to support DNS resolve and https with fetch.

[update]: all the test cases in js_fetch.t and js_fetch_objects.t passed.

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.
2 participants
@hongzhidao @xeioex