apigee-127 / swagger-node-runner

The heart of Swagger-Node
MIT License
102 stars 124 forks source link

compeating efforts to JSON.stringify ctx.output #73

Open osher opened 7 years ago

osher commented 7 years ago

Edited: beautify and used more precise language

Hi. I noted compeating efforts between: json_error_handler.js#L42 json_error_handler.js#L60 and connect_middleware.js#L127

If the concern of serializing a model object to a string is a concern of the connect_middleware, then the json_error_handler should not stringify, but leave a model object (the Error, perhaps augmented with statusCode or so)

I came to this thought when I approached a requirement to format error responses with in a custom envelope that is accustomed in the corp I do the job for.

My initial thought was :

swagger-route-error:
 - name: json_error_handler
 - fitting/the-corp-envelope

swagger-router:
 - onError: swagger-route-error
 - ...

Then I noted that the error handler leaves a string. If I want to manipulate it - I'll have to parse it, where it was an object just one step before...

.

I began to believe that the safe serialization logic belongs to connect_middleware, and should be agnostic to wether it's an error or not

(I mean, serialization error will still raise statusCode to 500, but it's not unique to json_error_handler)

What do you think?

theganyo commented 7 years ago

I agree. Good catch.

osher commented 7 years ago

Well. meanwhile, we released this: https://github.com/osher/swagger-json-output I'm not completely comfortable with it, but it fits the current situation.

If you would like to merge it with fittings/json_error_handler and call it, say, fitting/json_output_handler it will be OK with me, I have not proposed it because I try to work out a bigger picture.

The part I'm not sure is what if a service produces not json. My thoughts for that is that there should be a generic swagger-output fitting, that will leave on ctx.output a string that matches the expected content-type. The magic is that swagger-output should require on server load-time all packages found in package.json:dependencies whos names match /^swagger-(.*)-output$/ (or /^swagger-output-(.*)$/, debatable...), each of which may registger a strategy that hanldes the type it's responsible for (either each such module exposes a factory, or exposes an init hook that expects interface with .register(xMime:RegEx, fFormatter:handler(body), donno. thoughts are rolling. Need to think if there should be support for async inits... although I can't see a reason now, I think we should keep this door open (wierd cases like encryption formatters that obtain enc settings asynchronously... 😛)

The reason I'm not comfortable with it is because it somehow competes with connect_middleware~translate(body, mime). If translate is a fallback - it should be hermetic, and not re-throw errors if serialization errors occur, for whatever protocol and whatever reasons. So it's employing the same code twice.

Please let me have your thoughts, it should help crystallize mine...