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.

Sign up for GitHub

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

Jump to bottom

example snippet for akka http java dsl: SecurityDirectives #20717

Merged
merged 1 commit into from Jun 19, 2016
Merged

example snippet for akka http java dsl: SecurityDirectives #20717

merged 1 commit into from Jun 19, 2016

Conversation

Hawstein
Copy link
Contributor

@Hawstein Hawstein commented Jun 4, 2016

Ref #20466

@akka-ci
Copy link

akka-ci commented Jun 4, 2016

Can one of the repo owners verify this patch?

@ktoso
Copy link
Member

ktoso commented Jun 4, 2016

OK TO TEST

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Jun 4, 2016
@akka-ci
Copy link

akka-ci commented Jun 4, 2016

Test FAILed.

ktoso
ktoso reviewed Jun 6, 2016
View reviewed changes
akka-http-core/src/main/java/akka/http/javadsl/model/HttpMessage.java
@@ -72,6 +73,11 @@
Self addHeaders(Iterable<HttpHeader> headers);

/**
* Returns a copy of this message with the given http credential header added to the list of headers.
*/
Self addCredentials(HttpCredentials credentials);
Copy link
Member

Choose a reason for hiding this comment

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

That's a good addition 👍
Since no-one else but us may extend HttpMessage we are allowed to add the mima filters to make this pass.

Failure of build was:

[info] akka-http-core: found 2 potential binary incompatibilities while checking against com.typesafe.akka:akka-http-core_2.11:2.4.2  (filtered 122)
[error]  * abstract method addCredentials(akka.http.javadsl.model.headers.HttpCredentials)java.lang.Object in interface akka.http.javadsl.model.HttpMessage#MessageTransformations is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.javadsl.model.HttpMessage#MessageTransformations.addCredentials")
[error]  * method addCredentials(akka.http.javadsl.model.headers.HttpCredentials)akka.http.scaladsl.model.HttpMessage in trait akka.http.scaladsl.model.HttpMessage is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.scaladsl.model.HttpMessage.addCredentials")

And yeah, we can add those filters, so please do so in MiMa.scala :)
Thanks a lot in advance!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have fixed it and verified with mimaReportBinaryIssues, correct me if there exists something wrong in my code, thanks!

@Hawstein Hawstein force-pushed the akka-http-java-dsl-example-snippets-security-directives branch from fcc5acb to ca12572 Compare June 7, 2016 03:05
@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Jun 7, 2016
@akka-ci
Copy link

akka-ci commented Jun 7, 2016

Test PASSed.

@drewhk
Copy link
Member

drewhk commented Jun 7, 2016

LGTM

johanandren
johanandren reviewed Jun 9, 2016
View reviewed changes
akka-docs/rst/java/code/docs/http/javadsl/server/directives/SecurityDirectivesExamplesTest.java
// check if user is authorized to perform admin actions,
// this could potentially be a long operation so it would return a Future
final Set<String> admins = new HashSet<>();
admins.add("Peter");
Copy link
Member

Choose a reason for hiding this comment

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

Since this is mutable it is not safe to access it inside of the completable future, needs to be immutable, or a threadsafe set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will fix it now.

@johanandren
Copy link
Member

LGTM except for the mutable user set part

@Hawstein Hawstein force-pushed the akka-http-java-dsl-example-snippets-security-directives branch from ca12572 to 11f0d1a Compare June 9, 2016 14:12
@Hawstein
Copy link
Contributor Author

Hawstein commented Jun 9, 2016

@johanandren updated according to your comment.

@johanandren
Copy link
Member

Great, thanks!

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jun 9, 2016
@akka-ci
Copy link

akka-ci commented Jun 9, 2016

Test PASSed.

@ktoso
Copy link
Member

ktoso commented Jun 18, 2016

LGTM however does not merge (needs a rebase), would you be able to rebase this PR so we can merge @Hawstein ?

@Hawstein
Copy link
Contributor Author

@ktoso absolutely!

@Hawstein Hawstein force-pushed the akka-http-java-dsl-example-snippets-security-directives branch from 11f0d1a to 223d994 Compare June 18, 2016 19:09
@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jun 18, 2016
@Hawstein
Copy link
Contributor Author

@ktoso Have rebased the PR.

@akka-ci akka-ci added the tested PR that was successfully built and tested by Jenkins label Jun 18, 2016
@akka-ci
Copy link

akka-ci commented Jun 18, 2016

Test PASSed.

@akka-ci akka-ci removed the validating PR is currently being validated by Jenkins label Jun 18, 2016
@ktoso
Copy link
Member

ktoso commented Jun 19, 2016

Thanks a lot! Sorry you had to rebase:)

@ktoso ktoso merged commit cc22ed4 into akka:master Jun 19, 2016
@Hawstein Hawstein deleted the akka-http-java-dsl-example-snippets-security-directives branch June 19, 2016 15:38
Hawstein added a commit to Hawstein/akka that referenced this pull request Jun 24, 2016
@Hawstein
+doc example snippet for akka http java dsl: SecurityDirectives (akka…
7db9011 
…#20717)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Hawstein @akka-ci @ktoso @drewhk @johanandren