-
Notifications
You must be signed in to change notification settings - Fork 24
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
MIM-869 Handle APIv2 + APIv3 requests (phoenix controller) #153
Conversation
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 PR looks great! There is one small issue with String.to_atom
use, but other then that, ready to be merged :)
@@ -5,7 +5,7 @@ defmodule MongoosePush.API.V2.ResponseEncoder do | |||
@behaviour MongoosePush.API | |||
alias MongoosePush.Service | |||
|
|||
@spec to_status(:ok | {:error, Service.error() | {:error, MongoosePush.error()}}) :: | |||
@spec to_status(:ok | {:error, Service.error()} | {:error, MongoosePush.error()}) :: |
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.
Nice catch!
@spec decode(%Deep.MixedNotification{}) :: MongoosePush.request() | ||
def decode(schema) do | ||
%{ | ||
service: String.to_atom(schema.service), |
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.
Don't we have a helper that does this better? Even if not, then let's use to_existing_atom
at least.
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 implemented one, following the way @michalwski already suggested here.
@spec decode(%Deep.SilentNotification{}) :: MongoosePush.request() | ||
def decode(schema) do | ||
%{ | ||
service: String.to_atom(schema.service), |
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.
Don't we have a helper that does this better? Even if not, then let's use to_existing_atom at least.
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.
same as above
This pull request is the intuitive continuation of MIM-868 story, with the main difference that here I cover handling of the requests specified by APIv2 & APIv3.
In the home stretch it also turned out that I needed to - kind of - rollback to previous schemas design, reintroducing
MixedNotification
definition. Source rationale is thatOpenApiSpex
framework is not doing well when it finds incoming request matching more than one schema definition.