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

Add a flag when decoded args are truncated #543

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@hamishforbes
Contributor

hamishforbes commented Aug 5, 2015

Currently there's no way to tell if ngx.decode_args, ngx.req.get_uri_args or ngx.req.get_post_args silently discard arguments without enabling debug logging.

This change adds a second boolean return value to indicate if arguments have been truncated.

@agentzh

This comment has been minimized.

Member

agentzh commented Aug 5, 2015

@hamishforbes An additional return value can break backward compatibility, for example,

foo(ngx.req.get_uri_args(), "some other things")

will not work as expected because the second argument now becomes the 3rd, leading to (kindy) hard-to-debug issues.

We need to figure out a better way without breaking existing user code.

@hamishforbes

This comment has been minimized.

Contributor

hamishforbes commented Aug 6, 2015

Ah, that is a good point, didn't think of that!

We could add another argument that toggles returning this flag, e.g. local args, truncated = ngx.req.get_uri_args(100, true) but thats not particularly elegant either... what do you think?

The main issue is the args get truncated silently which can lead to odd issues when passing on request to other systems.
So maybe just raising the log level of the existing message to error or warn would be sufficient?

I think it would be preferable to allow the user to handle it themselves rather than log something but at least it would be clear what has happened.

@agentzh

This comment has been minimized.

Member

agentzh commented Aug 6, 2015

@hamishforbes You beat me :)

Another flag argument looks artificial to me as well. I would have a hard time documenting this argument if we implemented it.

Raising default error log level for the logging can open a door for malicious clients to flush your nginx error logs which can be very expensive because error logging is never buffered (for obvious reasons). Maybe adding a new configuration directive to allow the user to change the level? Still a bit cumbersome but a bit better than messing up the Lua API :) What do you think? (Sorry)

@hamishforbes

This comment has been minimized.

Contributor

hamishforbes commented Aug 6, 2015

Do you mean adding an option similar to lua_socket_log_errors?
That sounds reasonable.

The only other approach I can think of would be to add something like ngx.get_last_args_parse_error().
But thats not very nice either and smacks a bit of PHP...

@agentzh

This comment has been minimized.

Member

agentzh commented Aug 6, 2015

@hamishforbes Oh, seems like there's another way. How about adding an empty metatable to the resulting table in case of truncation? This won't break existing code I suppose and you have a way to handle this case in Lua yourself.

@hamishforbes

This comment has been minimized.

Contributor

hamishforbes commented Aug 6, 2015

I'm not entirely sure I follow, do you mean setting the metatable to {} rather than nil?

@agentzh

This comment has been minimized.

Member

agentzh commented Aug 6, 2015

@hamishforbes

This comment has been minimized.

Contributor

hamishforbes commented Aug 6, 2015

That sounds like a better solution.

I'm pretty new to the Lua C API but am I right in thinking this is as simple as

lua_newtable(L);
lua_setmetatable(L, top);
@agentzh

This comment has been minimized.

Member

agentzh commented Aug 7, 2015

@hamishforbes Instead of creating a new table upon every call, we could reuse and share a single empty table. It's supposed to be read-only anyway.

@hamishforbes

This comment has been minimized.

Contributor

hamishforbes commented Aug 7, 2015

Whats the best way to go about doing that? Or is there somewhere else in the codebase you've already used that technique?
Is it as simple as using lua_setglobal and lua_get_global?

@agentzh

This comment has been minimized.

Member

agentzh commented Aug 7, 2015

@hamishforbes hamishforbes force-pushed the hamishforbes:master branch from 5bf8b41 to 018ca5a Aug 7, 2015

@hamishforbes hamishforbes force-pushed the hamishforbes:master branch from 018ca5a to 79ba1f6 Aug 7, 2015

@hamishforbes

This comment has been minimized.

Contributor

hamishforbes commented Aug 7, 2015

Ah missed that one! Sounds like repeatedly calling luaL_newmetatable doesn't create a new table each time, so that should be fine?

Do you have any preference for naming of metatables like this, there doesn't seem to be any others in the current codebase.

I'll update and push tests/docs if you're happy with this approach.

@agentzh

This comment has been minimized.

Member

agentzh commented Aug 7, 2015

@hamishforbes Yes. Please go ahead :) Though I'm still not 100% happy with this approach ;)

@hamishforbes

This comment has been minimized.

Contributor

hamishforbes commented Aug 7, 2015

Ok cool, thanks for the pointers!

@agentzh agentzh force-pushed the openresty:master branch 2 times, most recently from e7ac10c to cfd4f90 Jan 31, 2016

@agentzh agentzh force-pushed the openresty:master branch from ab60f70 to 397f366 Oct 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment