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

chore: More detailed logging for ignored handling #20007

Merged
merged 2 commits into from
Sep 23, 2024
Merged

chore: More detailed logging for ignored handling #20007

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e010409
chore: More detailed logging for ignored handling
caalador Sep 20, 2024
0d3dffe
Add javadoc
caalador Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions flow-server/src/main/java/com/vaadin/flow/component/ComponentUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.vaadin.flow.internal.StateTree;
import com.vaadin.flow.internal.nodefeature.ElementData;
import com.vaadin.flow.internal.nodefeature.VirtualChildrenList;
import com.vaadin.flow.router.Route;
import com.vaadin.flow.router.Router;
import com.vaadin.flow.server.Attributes;
import com.vaadin.flow.server.VaadinService;
Expand Down Expand Up @@ -723,4 +724,14 @@ public static Router getRouter(HasElement component) {
return router;
}

public static Optional<Component> getRouteComponent(Component component) {
Copy link
Member

Choose a reason for hiding this comment

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

JavaDoc missing from a public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe not for this PR, but we have a findAncestor(Class) method in Component class.
It could be useful to have also a findAncestor(Predicate<Component>) for similar cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps if there comes up a need for this.

if (component.getClass().isAnnotationPresent(Route.class)) {
return Optional.of(component);
}
if (component.getParent().isPresent()) {
return getRouteComponent(component.getParent().get());
}
return Optional.empty();
}

}
46 changes: 39 additions & 7 deletions .../src/main/java/com/vaadin/flow/server/communication/rpc/AbstractRpcInvocationHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,14 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.ComponentUtil;
import com.vaadin.flow.component.PollEvent;
import com.vaadin.flow.component.UI;
import com.vaadin.flow.dom.Element;
import com.vaadin.flow.internal.StateNode;
import com.vaadin.flow.internal.nodefeature.ElementData;
import com.vaadin.flow.router.Route;
import com.vaadin.flow.shared.JsonConstants;
import elemental.json.JsonObject;

Expand Down Expand Up @@ -59,21 +64,48 @@ public Optional<Runnable> handle(UI ui, JsonObject invocationJson) {
// ignore RPC requests from the client side for the nodes that are
// invisible, disabled or inert
if (node.isInactive()) {
getLogger().info("Ignored RPC for invocation handler '{}' from "
+ "the client side for an inactive (disabled or invisible) node id='{}'",
getClass().getName(), node.getId());
logHandlingIgnoredMessage(node, "inactive (disabled or invisible)");
return Optional.empty();
} else if (!allowInert(ui, invocationJson) && node.isInert()) {
getLogger().info(
"Ignored RPC for invocation handler '{}' from "
+ "the client side for an inert node id='{}'",
getClass().getName(), node.getId());
logHandlingIgnoredMessage(node, "inert");
return Optional.empty();
} else {
return handleNode(node, invocationJson);
}
}

private void logHandlingIgnoredMessage(StateNode node, String reason) {
if (node != null && node.hasFeature(ElementData.class)) {
Element element = Element.get(node);
Optional<Component> component = element.getComponent();
StringBuilder targetInfo = new StringBuilder();
targetInfo.append("element with tag").append("'")
.append(element.getTag()).append("'");
if (component.isPresent()) {
targetInfo.append(" Component: ").append("'")
.append(component.get().getClass().getName())
.append("'");
Optional<Component> routeComponent = ComponentUtil
.getRouteComponent(component.get());
if (routeComponent.isPresent()) {
targetInfo.append(" Route: ").append("'")
.append(routeComponent.get().getClass()
.getAnnotation(Route.class).value())
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this will not work with dynamically registered routes using the RouteConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No that would need checking each parent against the registry which sounds like too much for a default info log message. Perhaps if running a trace log ...

.append("'");
}
}
getLogger().info(
"Ignored RPC for invocation handler '{}' from "
+ "the client side for an {} node id='{}' {}",
getClass().getName(), reason, node.getId(), targetInfo);
Copy link
Member

Choose a reason for hiding this comment

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

Both if-else blocks ends with mostly duplicate lines of code so it can be moved out side the if-else blocks in the end of the method and targetInfo could be blank by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

} else {
getLogger().info(
"Ignored RPC for invocation handler '{}' from "
+ "the client side for an {} node id='{}'",
getClass().getName(), reason, node.getId());
}
}

/**
* Checks whether a Poll RPC invocation is valid or not.
*
Expand Down
Loading