-
Notifications
You must be signed in to change notification settings - Fork 167
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
chore: More detailed logging for ignored handling #20007
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -59,21 +64,43 @@ 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) { | ||
StringBuilder targetInfo = new StringBuilder(); | ||
if (node != null && node.hasFeature(ElementData.class)) { | ||
Element element = Element.get(node); | ||
Optional<Component> component = element.getComponent(); | ||
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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
||
/** | ||
* Checks whether a Poll RPC invocation is valid or not. | ||
* | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
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 inComponent
class.It could be useful to have also a
findAncestor(Predicate<Component>)
for similar casesThere was a problem hiding this comment.
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.