root == '' gives access to the whole filesystem

[x-yuri]

...and the example is confusing:

Agoo::Server.init(6464, 'root')

This should work if the document root is at ./root I guess. But the first time I saw it I tried to not pass the argument (I didn't need to serve files from the filesystem). But it appeared that it's required. Tried to pass nil. Which appeared a non-option. So I settled with ''. Only to find out later that I don't want to do that. I suggest to allow to pass nil. After all it's already a legal value.

And as I said 'root' is confusing. "Does it really mean the document root?" I thought. To make it more clear you can use: './root', 'public_html', 'docroot'.

[ohler55]

Please try the "nil-root-okay" branch to start with. It allows a nil value for the root.

As for changing the name of the second argument in the documentation I can certainly add another sentence indicating some people prefer other terms such as "public_html" or "assets" but I'm not sure that would really make the documentation any more clear. You did read the documentation, right?

[x-yuri]

Nice. Now with nil it doesn't give access to the whole filesystem. I wonder if '' also has to be handled this way. Because giving access to the whole filesystem seems like a dangerous behavior.

Configures the server that will listen on the designated port and using the root as the root of the static resources.

https://rubydoc.info/gems/agoo/Agoo/Server#init-class_method

I believe "document root" is a more conventional term. "Root" sounds more vague.

About the name of the directory (root), it's no so about the name itself, as about the fact that it made me wonder if that's a path, a user name, or something else entirely. I believe prepending ./ will make it more clear (e.g. ..., './root')). Or using names other than root.

Of course I might be in the minority here. I can speak only for myself. To me "document root" and something other than 'root' (the argument) sound more clear.

Also it might make sense to document what exactly a handler can be. Although if it's just a string or an object with a call method (class, proc, lambda), maybe the way it is is okay.

And while you're at it, doesn't it make sense to make the root parameter optional? I don't think everybody needs to serve static files.

As for the documentation, I'm more used to projects where the documentation is in the README, some doc directory or an external website like Read the Docs or something custom-made. I rarely use rubydoc.info, probably because autogenerated documentation doesn't sound like a good first choice when starting out (starting to use a project). And if README or all that other stuff is not enough, I look in the code. Where you can also find the comments. But maybe I should try to use rubydoc.info more than I'm used to.

Anyways it didn't occur to me to look at the documentation for Agoo::Server. Maybe because I also wanted to figure out how serving static files and handling other requests interact with each other. Namely, what comes first or something. (Or the question arose further down the road?.. Anyways there was something I wasn't sure I understood.) Although trying the handle patterns first looked like a natural choice, I armed myself with gdb and inspected the code a bit. And somewhere in the middle I found that init is documented. But it didn't say document root, so for better or worse that didn't stop me.

Maybe I'm getting a bit paranoid lately. I occasionally need to spend some time with a debugger when starting to use a new tool. Probably when there are things I'm not sure I understand, the endeavor sounds relatively easy, and I have time. Or maybe it's not about being paranoid, but because I have this option, to dig deeper to confirm things.

On a side note

https://github.com/ohler55/agoo/blob/fa377390e08fb087a7794d067901cd2f0a5838db/ext/agoo/page.c#L857-L859

Shouldn't this be like (pseudocode):

if (!ends_with(cache.root, '/') && !starts_with(path, '/')) {

rather than:

if (!starts_with(cache.root, '/') && !starts_with(path, '/')) {

Unless I'm missing something.

[ohler55]

You ask a few questions in the above. I'll answer in pieces.

Where exactly do you think ./root should be used? Clearly no as a variable name so you must have something else in mind.

The handle() method on the server Takes a method, pattern, and handler. I can expand the documentation if it's not clear that a handler is not a Ruby object that implements the "method" specified.

I was going to say that RubyGems is where many people go to read documentation but when going to check the landing page for Agoo I noticed that the documentation link is missing so I need to figure out why it's not being generated. It's not reasonable to put all the documentation in the README.md file. There is too much of it and it is nicer to have a nicer way to navigate the docs other than a single very scroll.

I think most people would like a place for static assets and with the option for setting the root (document or asset root) I'm inclined to leave it as is.

I'd advise that just reading the C code is better than using GDB to look at the code but I guess if you are trying to interrupt at specific points GDB would be a good way to pinpoint where the execution is.

Thanks for the comment on the cache.root. Your suggestion isn't quite right but the check for path should be done later.

[x-yuri]

About the name of the directory (root), it's no so about the name itself, as about the fact that it made me wonder if that's a path, a user name, or something else entirely. I believe prepending ./ will make it more clear (e.g. ..., './root')). Or using names other than root.

Where exactly do you think ./root should be used? Clearly no as a variable name so you must have something else in mind.

Sorry if I didn't make that clear. I meant examples in the README. There you pass 'root' as an argument to Agoo::Server.init.

I'd advise that just reading the C code is better than using GDB to look at the code

I guess I don't know C (or the project) well enough to just read the code. With GDB it's usually easier.

The handle() method on the server Takes a method, pattern, and handler. I can expand the documentation if it's not clear that a handler is not a Ruby object that implements the "method" specified.

From the source code it looks like a handler can be one of three types of objects:

• respond_to? :on_request
• respond_to? :call (class, object, proc, lambda)
• respond_to? :create && respond_to? :read && respond_to? :update && respond_to? :delete

Also a string can be passed that should resolve to a class name, that falls into one of those 3 categories.

If it were only objects that respond to call or strings that resolve to a class that has a call method, then that would probably be okay. But the other 2 cases are not obvious. And I don't know what WAB::Controller is. And now that I think about it, if it does SomeClass.respond_to? :call then call must be a singleton (class) method. But the examples has an instance method, so it must check SomeClass.new.respond_to? :call. But such a check wouldn't work with a proc or lambda. Unless rb_respond_to() is smarter than the Ruby-facing method. See, my reading of the code has its limits. I need a debugger to figure it out. I'm totally missing something here.

It's not reasonable to put all the documentation in the README.md file.

Of course. It's reasonable to put enough to get one started. Which apparently means different things to different people. The downside of rubydoc.info is that you often don't know which part to read unless you know the project well enough. With README.md everything is laid out in a one-topic-after-another matter. I'm not saying that rubydoc.info is bad. But usually you want some introduction and only then reference.

Thanks for the comment on the cache.root. Your suggestion isn't quite right but the check for path should be done later.

That's strange. I can even reproduce it. If root is /agoo and I send:

GET a.rb HTTP/1.1

with telnet, full_path is /agooa.rb and it returns 404. The code clearly puts a separator between cache.root and path. And the fact that you check if cache.root starts, not ends with / looks like an error. But yeah I might miss something.

[ohler55]

The README.md has been updated in the branch mentioned earlier to make the example "./root".

The docs for the server handle method was also update. I realized when I went back and looked I had gotten it wrong in my description so clearly it needed updating.

The path / issue has fixed (I think). Basically checking the last byte written into the fullpath. I suppose that works out to the same thing you were saying but just from a different perspective. That is looking at the full path as it is constructed versus looking at what was used to construct it.

Thanks for your efforts. I'm still not sure what the best solution is for alternate forms of the documentation.

[ohler55]

I added a few links to the README.md file that refer to walk through examples in the misc directory. I'll be updating the https://www.ohler.com/agoo/doc/index.html once what ever changes are complete for this issue.

[ohler55]

Do you have any comments? If not I'll merge and release.

[x-yuri]

Sorry for the delay. Actually I'm going to try to inspect that part with *s++ = '/' once more, if you can wait a bit longer.

By the way don't you think that Agoo::Server.init() accepting an empty string for the root parameter is dangerous (gives access to the whole filesystem)? I mentioned it in the beginning. I believe making '' behave as nil is a better behavior.

Also I looked in the docs and noticed that there are 2 GraphQL pages there. When I click the first one, the menu item is active for a second, then switches to the second menu item. When I click the second one it gives 404.

And I think it should be explained somewhere what is WAB. Because google didn't help me with this. Doesn't look like it's a widely known term.

[ohler55]

I can wait.

I think you will find that a root of "" is now the same as nil.

Odd about the docs. I'll get it fixed.

Wab was an attempt to make an easier and higher performance approach to building web services. It never really caught on though.

[ohler55]

Duplicate GraphQL in menu fixed.

[ohler55]

For info on WABuR check out the link https://github.com/ohler55/wabur

[x-yuri]

I think you will find that a root of "" is now the same as nil.

Indeed.

Wab was an attempt to make an easier and higher performance approach to building web services.

Maybe you should give a link to the gem or say "see the wabur gem" if you mention WAB in the docs.

You might know about it, but in case you don't, agoo.gemspec is now broken. You can't + array and a string, and misc/glue.svg doesn't exist.

Actually I'm going to try to inspect that part with *s++ = '/' once more, if you can wait a bit longer.

Looks good now. Actually there's a similar issue in the sibling if branch :) (If you're going to make sure there's a slash between two strings, it's not clear why you care about the beginning of the first one.)

I'm still not sure what the best solution is for alternate forms of the documentation.

I guess you mean what to put into README... I don't know generally (what would users generally expect to see there?), I can only tell you about my cases. If you think they are valid, common and not too obvious, you can put into README information to cover them.

We're migrating to kubernetes and I was thinking about extracting the geoip stuff into a separate service. This case I guess is covered by the Rack example.

Also I'm thinking of switching from puma to something... more performant? I'm just not sure if puma with its workers and threads is a good fit for that kind of environment. And I was thinking if I could feed agoo config.ru. I've already found the solution in the example dir, but you might want to move or copy it into README.

Maybe there's not much to put into README in case of this project after all. I just don't know it well enough to tell if that is so.

[ohler55]

Thanks for hammering on Agoo.

A link to wabur was added.

The gemspec was fixed.

You are right about the root start. That was removed.

Most of the repos for gems don't put all the docs in the README.md file. For Agoo that would end up being something like 50 screenfuls or more. I think it's better to provide links and let people follow them.

[x-yuri]

Then I guess the issue can be closed. I'd like to help with the docs. I believe it can be improved. Not necessarily the README. But I'm not sure I'll find time :(

[ohler55]

I'd be glad to have some help on the docs. Maybe a little at a time when you find the time. Let me know what you have in mind first and we can make it a joint effort.

[ohler55]

Released v2.15.11