-
-
Notifications
You must be signed in to change notification settings - Fork 232
Refactor library interface #11
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
Conversation
| return len(c.errors) > 0 | ||
| } | ||
|
|
||
| func (c *hcontext) WriteHeader(status int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never understood why methods with this purpose are called "WriteHeader" in golang-world. You're not writing a "header" (as in a request or response header), you are writing a status code (or status line). I find the name confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I guess this implements http.ResponseWriter so 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is purely to match up with the standard library http.ResponseWriter interface so we don't really control it 😢
error.go
Outdated
| // ErrorDetail provides details about a specific error. | ||
| type ErrorDetail struct { | ||
| Message string `json:"message,omitempty"` | ||
| Location string `json:"location,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of Location? Specifically, is it a source code location? Because an end-user won't be able to do anything with that information (other than providing it in a support ticket, in which case some sort of error code would probably be better anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some additional documentation. This is actually the location in the input as a JSONPath type link and can refer to request parameters (path/query/header) or the request body. For example:
{
"status": 400,
"title": "Bad Request",
"detail": "Request input did not pass validation",
"errors": [
{
"message": "Could not parse int",
"location": "path.thing-id",
"value": "abc"
},
{
"message": "Field is required",
"location": "body.tags[1].description"
},
{
"message": "Must match pattern ^[a-z0-9-]+$",
"location": "body.slug",
"value": "foo_bar"
}
]
}| // NewRouter creates a new test router. It includes a logger attached to the | ||
| // test so if it fails you will see additional output. There is no recovery | ||
| // middleware so panics will get caught by the test runner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually find that very annoying, because I think that if a single test fails you'll get log spew from all the tests (making debugging harder). This would be even worse for table-drive tests where maybe just one case is failing. I would like to be able to at least have the option to not attach a logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a couple of options for this:
- Call
humatest.NewRouterfor each test in the table, then you get only the single failing log output. - Ignore the
humatestpackage and just dohuma.Newinstead and you get a blank router without any middleware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any examples of this in the existing code? I'd be good to see a setup that could be used to demonstrate this for future testing.
| ) | ||
|
|
||
| // OpenTracing provides a middleware for cross-service tracing support. | ||
| func OpenTracing(next http.Handler) http.Handler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to see an example of how this integrates with the datadog tracer API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is all you have to do:
// Start a Datadog tracer, optionally providing a set of options,
// returning an opentracing.Tracer which wraps it.
t := opentracer.New(tracer.WithAgentAddr("host:port"))
// Use it with the Opentracing API. The (already started) Datadog tracer
// may be used in parallel with the Opentracing API if desired.
opentracing.SetGlobalTracer(t)https://github.com/DataDog/dd-trace-go/blob/v1/ddtrace/opentracer/example_test.go#L15-L23
Co-authored-by: Marcin Dobosz <marcind@users.noreply.github.com>
| ## Resources | ||
|
|
||
| Huma APIs are composed of resources and sub-resources attached to a router. A resource refers to a unique URI on which operations can be performed. Huma resources can have dependencies, security requirements, parameters, response headers, and responses attached to them which are all applied to every operation and sub-resource. | ||
| Huma APIs are composed of resources and sub-resources attached to a router. A resource refers to a unique URI on which operations can be performed. Huma resources can have middleware attached to them, which run before operation handlers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which run before operation handlers.
Is my understanding correct that the middleware executes to completion, then the operation handler is executed?
No change requested, just looking to understand: I am mentally contrasting it with a gRPC interceptor where the middleware calls the handler so it can also take action on the handler response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same concept. Each middleware takes a next handler and calls next.ServeHTTP(w, r) on it, and at the end of the chain next is just the handler function itself.
Co-authored-by: Logan Garrett <34461561+lgarrett-isp@users.noreply.github.com>
| WriteModel(status int, model interface{}) | ||
| } | ||
|
|
||
| type hcontext struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a very informative type name; what is meant by hcontext?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name context is taken by a standard library package, so we append the name of this package. I guess that means it stands for "Huma context". It was just a way to prevent shadowing and is an internal implementation detail of the public huma.Context interface.
It's a pretty common pattern but by no means the only way to handle this. Open to other naming suggestions.
This change is a major refactor / partial rewrite of Huma which tries to accomplish a few things:
context.Contexthttp.ResponseWriterio.Readerio.Writerfunc(next http.Handler) http.HandlerSee the updated
README.mdfor more details.