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

Rework internal API #12

Merged
merged 25 commits into from
Jul 24, 2024
Merged

Rework internal API #12

merged 25 commits into from
Jul 24, 2024

Conversation

achouippe
Copy link
Contributor

@achouippe achouippe commented Jul 22, 2024

  • Add an OpenAPI descriptor for the internal API,
  • Support one or multiple topics in one HTTP call,
  • Support one or multiple published message in one HTTP cal,
  • Validation of the JSON input,
  • Split existing unit tests + comprehensive unit tests on the validation logic,
  • Possibility to pass message timestamps in the publish request.

@achouippe achouippe changed the title WIP - Comprehensive input validation Rework internal API Jul 23, 2024
@achouippe achouippe marked this pull request as ready for review July 23, 2024 15:30
@achouippe achouippe requested a review from bpaquet as a code owner July 23, 2024 15:30

Logger.debug("Message published on topic: #{topic}")
Stats.inc_msg_received()
Stats.inc_msg_received()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what we really want to monitor with the new API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we incr with nb_publish?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to count the number of call on the internal interface per issuer ?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, it should be outside of the loop :)

|> put_resp_header("content-type", "text/html")
|> send_resp(200, "Published #{message} to #{topic}\n")
|> put_resp_header("content-type", "application/json")
|> send_resp(200, "{\"nb_published\": #{nb_publish}}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Jason.encode?

}
end

def validate(message) do
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably write this with a list of guard. May be it's cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried first, but the combinatory effect make it much difficult to read.

end
end

def validate_topics(request) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, with guards?

end
end

def validate_messages(request) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Neurow.InternalApi.Message.from_json(payload["message"])

_ ->
nil
Copy link
Contributor

Choose a reason for hiding this comment

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

nil -> nil, but _ -> error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The validation is performed later, so I prefer not to return an error at this step.

neurow/lib/neurow/internal_api/endpoint.ex Show resolved Hide resolved

Logger.debug("Message published on topic: #{topic}")
Stats.inc_msg_received()
Stats.inc_msg_received()
Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, it should be outside of the loop :)

@achouippe achouippe merged commit 0faa1f9 into main Jul 24, 2024
3 checks passed
@achouippe achouippe deleted the rework-publish branch July 24, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants