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

Update branch #9665

Merged
merged 134 commits into from
Dec 16, 2020
Merged

Update branch #9665

merged 134 commits into from
Dec 16, 2020

Conversation

TatuLund
Copy link
Contributor

No description provided.

caalador and others added 30 commits October 26, 2020 12:32
Lock the registry when registering
dynamic route in serviceInit to not have
a race where 2 inits note that path is not
registered and then try to register the route.

Fixes #9223
8d012f7 removed some log and converted others into debug
but log in disconnectedCallback was left.
Exposes DataCommunicator::getDataProviderSize to the components, in order to get the items count taking into account the countCallback, if it has been set previously.

Related-to: vaadin/flow-components#282
Fix typo in DataCommunicator::getDataProviderSize() javadoc.
#9275)

HierarchicalDataCommunicator's expanded state were not being synchronised with client side and TreeGrid's expanded nodes were being collapsed after re-attaching.

Warranty: Sync TreeGrid expanded items state with the client side when detaching and reattaching

Fixes: #9175
* feat: Create Flow plugins for webpack

Moved stats file handling to a custom plugin.
Added feature for copying custom Flow plugins
for use with webpack.

Fixes #9283
AppShellConfigurator is now also considered when
scanning for annotations and deciding theme.

Fixes #9110
…der (#9336)

Fixes: #9328
Details: HierarchicalDataProvider's reset method was called before recreating the HierrarchyMapper in setDataProvider. This was creating wrong updates for the client side.
Having the `@Theme` annotation on Flow views or router layouts will not be allowed anymore, it should be on `AppShellConfigurator` instead.

Fixes #9092
…dated (#9329)

* test(TypeScript): ensure CSRF token is updated when session is invalidated

Fixes #9164

* add log message
- Use tree-shakeable ES imports for TS form validators based on the latest version of validators lib.
- Add missing `return` in the `submit()` method
Add a string definition for theme that matches the "application theme" in the theme folder inside /frontend/theme/, loading the css automatically from there. This is based on Lumo theme, always.
Change old class based theme to use themeClass for theme selection.

Fixes #9281
In-memory filtering and sorting are now stored directly in component, which gives an opportunity to change it through the data view API for a certain component separately from other components bound to the same data provider.

Fixes #8655
…ed (#9368)

Supports headers of the type Range: -123 and Range: 123-.

Fixes #9083.
…nent' (#9390)

Revert the changes for #8655 to be able to bump the version of components and avoid compile errors.
This module is subsumed by test-router-custom-context.
Theme Class and Theme Name is not supported in
the same Theme annotation as theme name builds
on the Lumo theme.

test-themes is now for testing the Application theme only.
Old theme test was moved into test-misc.

Fixes #9370
In-memory filtering and sorting are now stored directly in component, which gives an opportunity to change it through the data view API for a certain component separately from other components bound to the same data provider.

Fixes: #8655
Now we only handle the theme with the name inside the Theme annotation.

Fixes #9383
Read token file as a bundle resource in OSGi

Fixes #9146
Fix NPE
Revert logic back to the previous state
# Conflicts:
#	flow-server/src/main/java/com/vaadin/flow/server/DeploymentConfigurationFactory.java
#	flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java
#	flow-server/src/main/java/com/vaadin/flow/server/osgi/OSGiAccess.java
Fixes #9185
Exclude non-serializable classes
Drop dependency to flow-push from flow-server
# Conflicts:
#	flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java
#	flow-server/src/main/java/com/vaadin/flow/server/DeploymentConfigurationFactory.java
#	flow-server/src/main/resources/META-INF/services/javax.servlet.ServletContainerInitializer
#	flow-server/src/test/java/com/vaadin/flow/server/connect/generator/endpoints/model/OSGiInstantiatorFactory.java
#	flow-server/src/test/java/com/vaadin/flow/server/connect/generator/endpoints/model/OSGiResourceProvider.java
#	flow-server/src/test/java/com/vaadin/flow/server/connect/typeconversion/StringConversionTest.java
# Conflicts:
#	flow-push/pom.xml
#	flow-server/src/main/java/com/vaadin/flow/server/osgi/VaadinBundleTracker.java
theme = getFinder().loadClass(themeClass);
} else {
theme = getDefaultTheme();
if (theme == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule

initStandardLookup(classSet, servletContext);

DeferredServletContextInitializers initializers;
synchronized (servletContext) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR "servletContext" is a method parameter, and should not be used for synchronization. rule

*
*/
@HandlesTypes({ ResourceProvider.class, InstantiatorFactory.class,
DeprecatedPolymerPublishedEventHandler.class,
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 this use of "DeprecatedPolymerPublishedEventHandler"; it is deprecated. rule

* @see #DEFAULT_FLOW_RESOURCES_FOLDER
*/
@Deprecated
public static final String DEAULT_FLOW_RESOURCES_FOLDER =
DEFAULT_FLOW_RESOURCES_FOLDER;
public static final String DEAULT_FLOW_RESOURCES_FOLDER = DEFAULT_FLOW_RESOURCES_FOLDER;
Copy link
Collaborator

Choose a reason for hiding this comment

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

INFO Do not forget to remove this deprecated code someday. rule

getLogger().error("Failed to start the webpack process", e);
} catch (InterruptedException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Either re-interrupt this method or rethrow the "InterruptedException". rule

ClassLoader webClassLoader = ctx.getClassLoader();
ClassLoader classLoader = getClass().getClassLoader();
// see DeferredServletContextIntializers
DeferredServletContextInitializers.Initializer deferredInitializer = ctx -> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Reduce this lambda expression number of lines from 37 to at most 20. rule

} else {
/*
* The classloader which has loaded this class ({@code
* classLoader}) should be either the {@code webClassLoader}
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR This block of commented-out lines of code should be removed. rule

DeferredServletContextInitializers initializers = vaadinContext
.getAttribute(
DeferredServletContextInitializers.class,
() -> new DeferredServletContextInitializers());
Copy link
Collaborator

Choose a reason for hiding this comment

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

MINOR Replace this lambda with a method reference. rule

DeferredServletContextInitializers.class,
() -> new DeferredServletContextInitializers());
initializers.addInitializer(
ctx -> deferredInitializer.init(ctx));
Copy link
Collaborator

Choose a reason for hiding this comment

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

MINOR Replace this lambda with a method reference. rule

if (requiresLookup()) {
VaadinServletContext vaadinContext = new VaadinServletContext(
context);
synchronized (context) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR "context" is a method parameter, and should not be used for synchronization. rule

DevModeInitializer devModeInitializer = new DevModeInitializer();
devModeInitializer.onStartup(classes, servletContext);
waitForDevModeServer();
Thread.sleep(200);
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Remove this use of "Thread.sleep()". rule

* the token file data
* @return the config parameters
*/
public Map<String, String> getConfigParametersUsingTokenData(
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed. rule

}

private static String getTokenFileContents(Properties initParameters) {
private static String getTokenFileContents(Class<?> systemPropertyBaseClass,
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Remove this unused method parameter "systemPropertyBaseClass". rule


// Read the json and set the appropriate system properties if not
// already set.
if (json != null) {
JsonObject buildInfo = JsonUtil.parse(json);
setInitParametersUsingTokenData(initParameters, buildInfo);
// TODO : will be rewritten properly without extra instantiation
Copy link
Collaborator

Choose a reason for hiding this comment

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

INFO Complete the task associated to this TODO comment. rule

@@ -146,6 +133,7 @@ protected static Properties createInitParameters(
readUiFromEnclosingClass(systemPropertyBaseClass, initParameters);
readConfigurationAnnotation(systemPropertyBaseClass, initParameters);

// TODO : will be removed in futher commits
Copy link
Collaborator

Choose a reason for hiding this comment

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

INFO Complete the task associated to this TODO comment. rule


DevModeHandler.start(config, builder.npmFolder, runNodeTasks);
// Check whether executor is provided by the caller (framework)
Executor service = lookup.lookup(Executor.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR A "NullPointerException" could be thrown; "lookup" is nullable here. rule

}

private ResourceProvider getResourceProvider(BootstrapContext context) {
ResourceProvider resourceProvider = context.getSession()
Copy link
Collaborator

Choose a reason for hiding this comment

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

MINOR Immediately return this expression instead of assigning it to the temporary variable "resourceProvider". rule


@Override
public <U> U lookup(Class<U> serviceClass) {
if (services.contains(serviceClass)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR A "Set" cannot contain a "Class" rule

while (rangeMatcher.find()) {
final long start = Long.parseLong(rangeMatcher.group(1));
final long end = Long.parseLong(rangeMatcher.group(2));
Stack<Pair<Long, Long>> ranges = new Stack<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Replace the synchronized class "Stack" by an unsynchronized one such as "Deque". rule

if (end < start
|| (resourceLength >= 0 && start >= resourceLength)) {
// illegal range -> 416
getLogger().info("received an illegal range '{}' for resource '{}'",
rangeMatcher.group(), resourceURL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Invoke method(s) only conditionally. rule

if (startGroup.isEmpty() && endGroup.isEmpty()) {
response.setContentLengthLong(0L);
response.setStatus(416); // Range Not Satisfiable
getLogger().info("received a malformed range: '{}'", rangeMatcher.group());
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Invoke method(s) only conditionally. rule

@@ -125,51 +126,54 @@
*/
private URI nodeDownloadRoot = URI.create(NodeInstaller.DEFAULT_NODEJS_DOWNLOAD_ROOT);

private Lookup lookup;
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Make "lookup" transient or serializable. rule

}
VaadinContext context = ui.get().getSession().getService()
.getContext();
DeprecatedPolymerPublishedEventHandler handler = context
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 this use of "DeprecatedPolymerPublishedEventHandler"; it is deprecated. rule

.getContext();
DeprecatedPolymerPublishedEventHandler handler = context
.getAttribute(Lookup.class)
.lookup(DeprecatedPolymerPublishedEventHandler.class);
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 this use of "DeprecatedPolymerPublishedEventHandler"; it is deprecated. rule

@@ -180,14 +182,16 @@ private static void invokeMethod(Component instance, Method method,
Serializable returnValue = (Serializable) invokeMethod(instance,
method, args);

instance.getElement().executeJs("this.$server['"
instance.getElement()
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR A "NullPointerException" could be thrown; "instance" is nullable here. rule

@@ -107,13 +110,12 @@ public String getRpcType() {
PolymerServerEventHandlers eventHandlers = node
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 this use of "PolymerServerEventHandlers"; it is deprecated. rule

instance.getElement().executeJs("this.$server['"
+ JsonConstants.RPC_PROMISE_CALLBACK_NAME
+ "']($0, false)", Integer.valueOf(promiseId));
instance.getElement()
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR A "NullPointerException" could be thrown; "instance" is nullable here. rule

@@ -107,13 +110,12 @@ public String getRpcType() {
PolymerServerEventHandlers eventHandlers = node
.getFeature(PolymerServerEventHandlers.class);
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 this use of "PolymerServerEventHandlers"; it is deprecated. rule

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 84 issues

  • CRITICAL 3 critical
  • MAJOR 36 major
  • MINOR 36 minor
  • INFO 9 info

Watch the comments in this conversation to review them.

Top 10 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 TaskRunNpmInstall.java#L277: Reduce the number of conditional operators (4) used in the expression (maximum allowed 3). rule
  2. MAJOR DataCommunicator.java#L1070: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed. rule
  3. MAJOR NpmTemplateParser.java#L93: Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed. rule
  4. MAJOR NpmTemplateParser.java#L111: Reduce the total number of break and continue statements in this loop to use at most one. rule
  5. MAJOR ResponseWriter.java#L194: Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed. rule
  6. MAJOR ResponseWriter.java#L292: Format string should use %n rather than \n in com.vaadin.flow.internal.ResponseWriter.writeMultipartRangeContents(List, URLConnection, HttpServletResponse, URL) rule
  7. MAJOR ResponseWriter.java#L297: Format string should use %n rather than \n in com.vaadin.flow.internal.ResponseWriter.writeMultipartRangeContents(List, URLConnection, HttpServletResponse, URL) rule
  8. MAJOR ResponseWriter.java#L301: Format string should use %n rather than \n in com.vaadin.flow.internal.ResponseWriter.writeMultipartRangeContents(List, URLConnection, HttpServletResponse, URL) rule
  9. MAJOR ResponseWriter.java#L322: Format string should use %n rather than \n in com.vaadin.flow.internal.ResponseWriter.writeMultipartRangeContents(List, URLConnection, HttpServletResponse, URL) rule
  10. MAJOR DeploymentConfigurationFactory.java#L325: Remove this unused private "verifyFolderExists" method. rule

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet