Skip to content

refactor: Removed hard-coding of h2 console path and used H2ConsoleProperties i… #1306

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
Merged
merged 1 commit into from
Mar 31, 2020
Merged

refactor: Removed hard-coding of h2 console path and used H2ConsoleProperties i… #1306

merged 1 commit into from
Mar 31, 2020

Conversation

mscibilia
Copy link
Contributor

@mscibilia mscibilia commented Mar 28, 2020 •
edited
Loading

…nstead.

Addressed to #1269

@mscibilia mscibilia requested a review from php-coder as a code owner March 28, 2020 08:40
@mystamps-bot
Copy link

mystamps-bot commented Mar 28, 2020 •
edited
Loading

1 Message
📖 @mscibilia thank you for the PR! All quality checks have been passed! Next step is to wait when @php-coder will review this code

Generated by 🚫 Danger

@mscibilia mscibilia force-pushed the gh1269_Reduce-h2-console-path-hard-coding branch 2 times, most recently from f8da737 to d9608d3 Compare March 29, 2020 01:30
@php-coder php-coder mentioned this pull request Mar 29, 2020
Closed
php-coder
php-coder requested changes Mar 29, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I'd like to accept your contribution after we deal with small issues (see my comments).

Also, could you amend the commit message and start it with "refactor:" prefix (like PR's title). We have a bot that should control this but it seems like bot didn't recognize this :-(

Thank you again!

@mscibilia mscibilia force-pushed the gh1269_Reduce-h2-console-path-hard-coding branch from d9608d3 to ca97184 Compare March 30, 2020 08:53
@mscibilia
Copy link
Contributor Author

mscibilia commented Mar 30, 2020 •
edited
Loading

Thank you for the PR! I'd like to accept your contribution after we deal with small issues (see my comments).

Also, could you amend the commit message and start it with "refactor:" prefix (like PR's title). We have a bot that should control this but it seems like bot didn't recognize this :-(

Thank you again!

Hi @php-coder ,
I've made the changes :)
Please re-review

php-coder
php-coder requested changes Mar 30, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

Thanks again!

LGTM with 2 small improvements (I'm sorry for being so pedantic):

Thank you! 👍

@mscibilia mscibilia force-pushed the gh1269_Reduce-h2-console-path-hard-coding branch from ca97184 to 35f9f5f Compare March 30, 2020 09:43
@php-coder
Copy link
Owner

@mscibilia Github didn't recognize this keyword. I suppose because it should be on a separate line:

refactor: remove hard-coding of h2 console path and use H2ConsoleProperties instead.

Fix #1269

Could you modify it one more time, please?

Also, I sent you invite to join as a collaborator. If you accept it, I'll be able to assign the issue to you.

@php-coder
Copy link
Owner

P.S. @mscibilia I see that the commit has "Scibs" as an author name. If you want to modify it, you can do that with git config user.name "Your Name" && git commit --am --reset-author Otherwise I'll merge it as-is.

@mscibilia mscibilia force-pushed the gh1269_Reduce-h2-console-path-hard-coding branch from 35f9f5f to 4893ef9 Compare March 30, 2020 14:30
php-coder
php-coder approved these changes Mar 30, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for patience! 🥇

@php-coder
Copy link
Owner

I don't see a report from Travis CI on GitHub, but it seems like there some failures: https://travis-ci.org/github/php-coder/mystamps/builds/668788917 I'm looking on it right now.

php-coder
php-coder reviewed Mar 30, 2020
View reviewed changes
src/main/java/ru/mystamps/web/support/spring/security/SecurityConfig.java
@@ -72,6 +73,9 @@
@Autowired
private SiteService siteService;

@Autowired
private H2ConsoleProperties h2ConsoleProperties;
Copy link
Owner

Choose a reason for hiding this comment

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

The integration tests fail with error:

Field h2ConsoleProperties in ru.mystamps.web.support.spring.security.SecurityConfig required a bean of type 'org.springframework.boot.autoconfigure.h2.H2ConsoleProperties' that could not be found.

The injection point has the following annotations:

	- @org.springframework.beans.factory.annotation.Autowired(required=true)

They fail only on "travis" and "postgres" profiles. It happens because we use H2 only in "test" profile and it's disabled on others (including production where we use MySQL, for example).

Copy link
Owner

Choose a reason for hiding this comment

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

We need to mark this as required=true and handle null case later.

php-coder
php-coder reviewed Mar 30, 2020
View reviewed changes
src/main/java/ru/mystamps/web/support/spring/security/SecurityConfig.java Outdated
@@ -141,7 +145,7 @@ protected void configure(HttpSecurity http) throws Exception {
// Allow unsecured requests to H2 consoles.
// See also spring.h2.console.path in application-test.properties and
// ContentSecurityPolicyHeaderWriter.H2_CONSOLE_PATTERN
Copy link
Owner

Choose a reason for hiding this comment

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

This comment should be updated as well, by the way.

php-coder
php-coder reviewed Mar 30, 2020
View reviewed changes
src/main/java/ru/mystamps/web/support/spring/security/SecurityConfig.java Outdated
@@ -141,7 +145,7 @@ protected void configure(HttpSecurity http) throws Exception {
// Allow unsecured requests to H2 consoles.
// See also spring.h2.console.path in application-test.properties and
// ContentSecurityPolicyHeaderWriter.H2_CONSOLE_PATTERN
.ignoringAntMatchers("/console/**", SiteUrl.CSP_REPORTS_HANDLER)
.ignoringAntMatchers(h2ConsoleProperties.getPath() + "/**", SiteUrl.CSP_REPORTS_HANDLER)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's re-use h2ConsolePath local variable that has been introduced earlier.

php-coder
php-coder requested changes Mar 30, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

May I ask you to update PR based on my comments to fix the build on Travis?

Ping me when you will need me to review again.

@mscibilia mscibilia force-pushed the gh1269_Reduce-h2-console-path-hard-coding branch from 4893ef9 to 1038ad4 Compare March 31, 2020 10:59
@mscibilia
Copy link
Contributor Author

May I ask you to update PR based on my comments to fix the build on Travis?

Ping me when you will need me to review again.

Please re-review

php-coder
php-coder requested changes Mar 31, 2020
View reviewed changes
src/main/java/ru/mystamps/web/support/spring/security/SecurityConfig.java Outdated
http.csrf()
// Allow unsecured requests to H2 consoles.
// See also spring.h2.console.path in application-test.properties
.ignoringAntMatchers(h2ConsolePath + "/**", SiteUrl.CSP_REPORTS_HANDLER);
Copy link
Owner

Choose a reason for hiding this comment

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

I'd love to merge this is but I can't because this change introduces regression that fortunately was caught by our integration tests:

FAIL: Robotframework.Site.Csp.Report-Logic.CSP report should be accepted

Status code was not as expected. Expected 204, but got 405

Here we ignores 2 URLS -- one for H2 and another one for CSP reports. The first one should be ignored only when we have H2 while another one should be ignored always.

Let's leave it as it was before -- ignore h2 always and just add // FIXME comment that we should ignore H2 only when it's available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @php-coder,

I've pushed a solution, let me know what you think :)

Copy link
Owner

Choose a reason for hiding this comment

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

Looks great! 👍 I had the same idea :)

@mscibilia mscibilia force-pushed the gh1269_Reduce-h2-console-path-hard-coding branch from 1038ad4 to 1d813a0 Compare March 31, 2020 12:47
php-coder
php-coder requested changes Mar 31, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

CheckStyle complains on a trailing spaces and marks build as red. Everything else works. After you fix such a tiny issue, I'd be happy to merge this!

@php-coder
Copy link
Owner

P.S. The build log is available there: https://travis-ci.org/github/php-coder/mystamps/jobs/669211574

@mscibilia mscibilia force-pushed the gh1269_Reduce-h2-console-path-hard-coding branch from 1d813a0 to 13657e0 Compare March 31, 2020 13:07
@php-coder
Copy link
Owner

Hooray! It's green now: https://travis-ci.org/github/php-coder/mystamps/builds/669225988

php-coder
php-coder approved these changes Mar 31, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@php-coder php-coder merged commit 98c17a3 into php-coder:master Mar 31, 2020
@php-coder php-coder mentioned this pull request Mar 31, 2020
Closed
@php-coder
Copy link
Owner

@mscibilia Thank you, Matthew! I've merged your change and added you to our list of contributors -- https://github.com/php-coder/mystamps/wiki/team-members

Let me know if you want to work on something else. For example, #1307 is a follow-up issue that looks like a low-hanging fruit ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@php-coder php-coder php-coder approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mscibilia @mystamps-bot @php-coder