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

need orginal path to generate api docs #4084

Closed
MichelLiu opened this issue Oct 24, 2019 · 16 comments
Closed

need orginal path to generate api docs #4084

MichelLiu opened this issue Oct 24, 2019 · 16 comments
Labels

Comments

@MichelLiu
Copy link

I want to get all the router routing information by traversing app._route.stack and automatically generate the swagger json document in combination with joi. However, Layer Contructor directly parses the path into regexp, and I can't get the original path.

@dougwilson
Copy link
Contributor

@wesleytodd
Copy link
Member

Oh I posted in the closed PR with the same link! :)

@dougwilson, we never really landed on the final agreement, but if you are not opposed I could move that over to the express org and get it published under the scope. This topic currently has 3 or 4 related open issues which I could follow up on if we are in agreement that my solution to this works.

@MichelLiu
Copy link
Author

MichelLiu commented Oct 30, 2019

@dougwilson ,
We have used express online and cannot be replaced directly with express-openapi. I temporarily wrote this to reverse the regexp to the original path.

function getFullPath(layer) {
        let routeName = "";
        if (layer.regexp) {
            routeName = layer.regexp.source.replace("^\\","").replace("\\/?(?=\\/|$)","");
            if (layer.keys && layer.keys.length > 0) {
                layer.keys.forEach(function (key) {
                    routeName = routeName.replace("(?:([^\\/]+?))",":" + key.name);
                });
            }
        }
        return routeName.split("\\").join("");
}

@dougwilson
Copy link
Contributor

We have used express online and cannot be replaced directly with express-openapi

I'm not sure what this means. Can you ellaborate? Perhaps you can help us understand what you are trying to achieve in some way? For example, you can maybe provide a demo of what you would do with the given path?

@MichelLiu
Copy link
Author

MichelLiu commented Oct 30, 2019

eg.
//router.js

router.post('/blog', joiSwagger.joiValidator({
    summary:"blog",
    description: "博客",
    validators: {
        body: joi.object().keys({
            comboName: joi.string().when('role', { is: 'mainaccount', then: joi.string().required() }).when('role', { is: 'subaccount', then: joi.string().allow('') })
        })
    },
    responseExamples: [
        {
            code: 200,
            description: "normal response",
            schema: joi.object().keys({
                "error_code": joi.number().integer().default(0).description("错误码"),
                "message": joi.string().default("success").description("错误字符串"),
                "success": joi.boolean().default(true),
                "data": {}
            })
        }
    ]
}) ,function (req, res) {
    console.log('/api/blog');

    res.json(req.body);
});

//app.js
app.use("/api/:cb", require("./router"));

//this function needs to concat '/api/:cb' with '/blog' in order to generate swagger document
convertToSwaggerDoc(app._router.stack)

// function definition

stack.forEach(function (layer) {
  layer.handle.stack.forEach(function (subLayer) {
                    if (subLayer.route && subLayer.route.stack) {
                        subLayer.route.stack.forEach(function (middlewareFunction) {
                            if (middlewareFunction.name === "validateJoi") {
                                const options = middlewareFunction.routedef || middlewareFunction.handle.routedef;
                                const path = getFullPath(layer) + subLayer.route.path;
                                const method = layer.methods ? layer.methods[layer.methods.length-1].toLowerCase() : middlewareFunction.method;
                                thisPointer.addRouteDefinition(method, path);
                            }
                        });
                    }
                })
}

@dougwilson
Copy link
Contributor

So what you are trying to do is exactly what the module I linked to does. Why is it not usable, specifically?

@MichelLiu
Copy link
Author

We need a joiSwagger middleware that can do both joi parameter validation and convert express or eggjs app.stack to swagger documentation. We have already launched the middleware and proposed this issue just to reduce the operation of a path conversion. If I use express-openapi, I need to change a lot of code, which is out of date.

@dougwilson
Copy link
Contributor

dougwilson commented Oct 30, 2019

So the issue with your proposed solution would not really work any better; users can directly use regular expressions to define a path, so you're always going to have to reverse a regular expression back into the path. The reason we only keep the regular expression is because of the multiple ways in which a user can define an Express route, that is the least-common-denominator. If we add your proposed property to Express, what is your method to support regular expression paths?

@MichelLiu
Copy link
Author

As for the regular expression paths, we are directly converted to a form of toString. The normal path we show is normal.

@wesleytodd
Copy link
Member

@MichelLiu, to your specific issue: Express cannot add features just because one use case where you are combining other modules is difficult. I am 👎 on this PR because of that.

That said, I think this is a very common requirement, this issue is an example of the reason I think we should have a solution for this maintained directly for Express. Lack of it has led more than just @MichelLiu into a position like this (my current team and others I have talked with on the topic included).

@GuskiS
Copy link

GuskiS commented Oct 31, 2019

I have similar case - I need to group my request logs based on path, but since express transforms path and doesn't provide original path it is not possible.

@wesleytodd
Copy link
Member

@GuskiS, so that I am clear what you mean, you want to log the parameterized route (ex: /user/:id) along with your application logs so that you can trace logs together for which route triggered that code path?

If so, then I can see that this might be a more "general purpose" feature than specifically the "generate docs" use case. Depending on how you structure your app/routes you can get some unexpected results from req.route, but still seems like a reasonable ask.

All of this has made me take a look at what I did in the open api module, and it would actually simplify some code I wrote there if we did add some reference to the original "route" before path-to-regexp parses it.

That all said, I think if we want to move forward with this idea we should move the conversation over to the router repo.

@GuskiS
Copy link

GuskiS commented Nov 1, 2019

@wesleytodd Yes, that is exactly what I want to do.

@wesleytodd
Copy link
Member

Ok, can one of you open a new issue detailing this request well so that we can have a better discussion on the router repo? I will close this one, but feel free to link back to it from the new issue.

@dougwilson
Copy link
Contributor

I would also like to ask if we can better frame it around the underlying feature request and not just specifically justifying storing the original value. The reason is because I am very hesitant about adding that, as it seems like the general intention is then to build middleware and such around assuming the value there is just a single string without also properly handing regular expressions and arrays.

@GuskiS
Copy link

GuskiS commented Nov 4, 2019

Created issue: pillarjs/router#85

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants