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

Extract app config from deployment config #9642

Merged
merged 32 commits into from
Dec 22, 2020
Merged

Extract app config from deployment config #9642

merged 32 commits into from
Dec 22, 2020

Conversation

denis-anisimov
Copy link
Contributor

fixes #9417

@denis-anisimov denis-anisimov mentioned this pull request Dec 16, 2020
Merged
@denis-anisimov denis-anisimov force-pushed the 9417-introduce-app-config branch from 0714fa1 to 16157c4 Compare December 16, 2020 06:43
@denis-anisimov denis-anisimov changed the title WIP: extract app config from deployment config Extract app config from deployment config Dec 17, 2020
Denis Anisimov added 19 commits December 17, 2020 11:46
refactor: extract common config functionality and introduce add config
ea29c37 
part of #9417
refactor: use application level configuration instead of deployment conf
9c41873
chore: implement deployment configs being based on parent app config
e13980f
fix: fix javadoc params
d9fcad8
fix: correct javadocs references
7c8be83
test: use App config and adapt unit tests
b1e65f4
fix: correct variable name
37d1030
test: make unit tests for DefaultApplicationConfigurationFactory
b42bd48
test: add unit tests for properties in DefaultDeploymentConfiguration
85ce4fd
test: add precedence tests for PropertyDeploymentConfiguration
282fc3c
test: add unit test for app config factory in lookup
e119e09
test: extend unit tests for PropertyDeploymentConfiguration
352b98d
test: use App config in DevModeHandlerTest instead deployment config
e91f0be
test: use app config instead of deployment config in unit tests
81ea8f3
test: fixes unit tests after introducing merging properties
5e4a45d
test: fix DevModeInitializerClassLoaderTest
8cce983
fix: yet another fix of unit tests
1b773f5
test: run ITs without app shell validation check
92f993d
fix: yet another unit test fix
dda1b6e
@denis-anisimov denis-anisimov force-pushed the 9417-introduce-app-config branch from 150bb69 to dda1b6e Compare December 17, 2020 09:41
@denis-anisimov denis-anisimov force-pushed the 9417-introduce-app-config branch from bf1beb4 to 497b282 Compare December 17, 2020 10:59
@denis-anisimov denis-anisimov force-pushed the 9417-introduce-app-config branch from 5bb7589 to a221f98 Compare December 18, 2020 08:42
pleku
pleku reviewed Dec 18, 2020
View reviewed changes
Copy link
Contributor

@pleku pleku left a comment

Choose a reason for hiding this comment

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

Mostly questions

flow-server/src/main/java/com/vaadin/flow/server/DeploymentConfigurationFactory.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/DeploymentConfigurationFactory.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/PropertyDeploymentConfiguration.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/VaadinServletConfiguration.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/startup/AbstractConfigurationFactory.java Outdated Show resolved Hide resolved
flow-server/src/test/java/com/vaadin/flow/server/communication/UidlRequestHandlerTest.java Show resolved Hide resolved
...oint/src/test/java/com/vaadin/flow/server/startup/fusion/DevModeInitializerEndpointTest.java Show resolved Hide resolved
...oint/src/test/java/com/vaadin/flow/server/startup/fusion/DevModeInitializerEndpointTest.java Show resolved Hide resolved
...oint/src/test/java/com/vaadin/flow/server/startup/fusion/DevModeInitializerEndpointTest.java Show resolved Hide resolved
flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java Show resolved Hide resolved
pleku
pleku previously approved these changes Dec 21, 2020
View reviewed changes
Copy link
Contributor

@pleku pleku left a comment

Choose a reason for hiding this comment

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

Only one comment open about a test checking whether methods are overridden or not, your call @denis-anisimov if it makes sense or not

vaadin-bot
vaadin-bot reviewed Dec 21, 2020
View reviewed changes
flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java
@@ -180,14 +181,10 @@ public static VaadinServlet getCurrent() {
*/
protected DeploymentConfiguration createDeploymentConfiguration()
throws ServletException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MINOR Remove the declaration of thrown exception 'javax.servlet.ServletException', as it cannot be thrown from method's body. rule

@vaadin vaadin deleted a comment from pleku Dec 21, 2020
@vaadin vaadin deleted a comment from denis-anisimov Dec 21, 2020
@denis-anisimov
Copy link
Contributor Author

Only one comment open about a test checking whether methods are overridden or not, your call @denis-anisimov if it makes sense or not

I've added the test sine this may be the most popular mistake. It won't catch all the possible errors but at least some.

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 15 issues

  • CRITICAL 2 critical
  • MAJOR 9 major
  • MINOR 4 minor

Watch the comments in this conversation to review them.

9 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. CRITICAL DefaultApplicationConfigurationFactory.java#L175: Define a constant instead of duplicating this literal "jar!/" 3 times. rule
  2. MAJOR DefaultDeploymentConfiguration.java#L170: Merge this if statement with the enclosing one. rule
  3. MAJOR DevModeHandler.java#L554: Remove this unused private "checkPort" method. rule
  4. MAJOR DevModeHandler.java#L691: Either re-interrupt this method or rethrow the "InterruptedException". rule
  5. MAJOR DevModeInitializer.java#L207: Remove this unused private "isVaadinServletSubClass" method. rule
  6. MAJOR DevModeInitializer.java#L362: A "NullPointerException" could be thrown; "lookup" is nullable here. rule
  7. MAJOR LookupInitializer.java#L210: "servletContext" is a method parameter, and should not be used for synchronization. rule
  8. MAJOR DevModeHandlerTest.java#L559: Remove this use of "Thread.sleep()". rule
  9. MINOR VaadinServlet.java#L529: Replace this lambda with a method reference. rule

@denis-anisimov denis-anisimov removed the request for review from caalador December 21, 2020 10:21
@pleku pleku merged commit e3821fd into master Dec 22, 2020
@pleku pleku deleted the 9417-introduce-app-config branch December 22, 2020 08:41
pleku added a commit to vaadin/cdi that referenced this pull request Dec 23, 2020
@pleku
refactor: fix unit tests after app-config changes
88a83e9 
Adds necessary mocking for application configuration for CdiServlet
in unit tests.

Related to vaadin/flow#9642
@pleku pleku mentioned this pull request Dec 23, 2020
Merged
caalador added a commit that referenced this pull request Dec 23, 2020
@caalador
Revert "refactor!: Extract app config from deployment config (#9642)"
b411d66 
This reverts commit e3821fd.

# Conflicts:
#	flow-server/pom.xml
#	flow-server/src/main/java/com/vaadin/flow/server/startup/DefaultApplicationConfigurationFactory.java
#	flow-tests/test-root-context/src/main/java/com/vaadin/flow/ItApplicationConfigurationFactory.java
pleku added a commit to vaadin/cdi that referenced this pull request Dec 23, 2020
@pleku
refactor: fix unit tests after app-config changes
ee39f39 
Adds necessary mocking for application configuration for CdiServlet
in unit tests.

Related to vaadin/flow#9642
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaadin-bot vaadin-bot vaadin-bot left review comments

@pleku pleku pleku approved these changes

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

Successfully merging this pull request may close these issues.

Make StubServletConfig private and remove every usage this class in the code outside of ServletDeployer
3 participants
@denis-anisimov @vaadin-bot @pleku