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

Supports HAPI 17 API #7

Merged
merged 3 commits into from Feb 14, 2018
Merged

Supports HAPI 17 API #7

merged 3 commits into from Feb 14, 2018

Conversation

joseluisdiaz
Copy link
Member

@joseluisdiaz joseluisdiaz commented Jan 29, 2018

HAPI version 17 does not support the concept of connection hapijs/hapi#3658 because of this what server.table() returns changed

Because of this is the major change, and the flattening of schemasByKey, previous schemasByServerUri. Could ve possible replace this idea with vhost and use it to create the key.

Since the HAPI new API, introduces breaking changes this update does not escape that therefore, this is a mayor version update.

}
catch (error){
log('error', 'Unable to compile and validate schemas', error);
return reply(error);
return error;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know if it's possible to return a boom error on hapi 17, but maybe it's better to throw it? It's a bit confusing to return error imo.

@@ -48,14 +45,14 @@ var RequestValidator = function(plugin, options) {
report = routeSchemaManager['validate' + capitalizedParts](request);

if (!report.valid){
return reply(Boom.badRequest(configuredErrorReporters[parts[i]].extractMessage({ report: report, context: { request: request } })));
return Boom.badRequest(configuredErrorReporters[parts[i]].extractMessage({ report: report, context: { request: request } }));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

}
return reply(Boom.internal(errorMessages));
return Boom.internal(errorMessages);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

lib/ratify.js Outdated
}
else {
reply(Boom.notFound());
return Boom.notFound();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

@@ -6,7 +6,7 @@ var ZSchema = require('z-schema'),
var RouteSchemaManager = function(options) {
options = options || {};

var schemasByServerUri = {},
var schemasByKey = {},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because hapi does not support multiple connections anymore, right?

};
});
var get = function (request, h) {
return { clientId: 'fnord', email: 'myemail@fake.com' };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h is not used.

@dafortune
Copy link

We don't want to handle all the problems at the same time so 👍 on doing the minimal changes, although var is weird... 👍
Do you know what are the differences between this plugin and the original version? maybe they have already added the features we need?

@dafortune dafortune merged commit 0752818 into auth0:master Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants