Copilot commented on code in PR #264: URL: https://github.com/apache/maven-dependency-analyzer/pull/264#discussion_r2733182787
########## src/main/java/org/apache/maven/shared/dependency/analyzer/dependencyclasses/WarMainDependencyClassesProvider.java: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.shared.dependency.analyzer.dependencyclasses; + +import javax.inject.Named; +import javax.inject.Singleton; +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +import java.io.File; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +import org.apache.maven.model.Plugin; +import org.apache.maven.project.MavenProject; +import org.apache.maven.shared.dependency.analyzer.ClassesPatterns; +import org.apache.maven.shared.dependency.analyzer.DependencyUsage; +import org.apache.maven.shared.dependency.analyzer.MainDependencyClassesProvider; +import org.codehaus.plexus.util.xml.Xpp3Dom; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; +import org.xml.sax.SAXException; + +/** + * Implementation of {@link MainDependencyClassesProvider} for web applications. + */ +@Named +@Singleton +class WarMainDependencyClassesProvider implements MainDependencyClassesProvider { + + private static final Logger LOGGER = LoggerFactory.getLogger(WarMainDependencyClassesProvider.class); + + @Override + public Set<DependencyUsage> getDependencyClasses(MavenProject project, ClassesPatterns excludedClasses) + throws IOException { + if (!"war".equals(project.getPackaging())) { + return Collections.emptySet(); + } + + File webXml = findWebXml(project); + if (webXml == null) { + LOGGER.debug("No web.xml found for project {}", project); + return Collections.emptySet(); + } + + if (!webXml.isFile()) { + LOGGER.debug("{} is not a file in project {}", webXml, project); + return Collections.emptySet(); + } + + try { + return processWebXml(webXml, excludedClasses); + } catch (SAXException | ParserConfigurationException e) { + LOGGER.warn("Error parsing web.xml file {}: {}", webXml, e.getMessage()); + return Collections.emptySet(); + } + } + + private File findWebXml(MavenProject project) { + // standard location + File webXmlFile = new File(project.getBasedir(), "src/main/webapp/WEB-INF/web.xml"); + if (webXmlFile.isFile()) { + return webXmlFile; + } + + // check maven-war-plugin configuration for custom location of web.xml Review Comment: Potential NullPointerException: project.getBuild() can return null. Consider adding a null check before calling getPluginsAsMap(), similar to how it's handled in the test at line 74 where an empty Build is explicitly set. If getBuild() returns null, the method should return null early to avoid the NPE. ```suggestion // check maven-war-plugin configuration for custom location of web.xml if (project.getBuild() == null) { LOGGER.debug("No build found for project {}", project); return null; } ``` ########## src/test/java/org/apache/maven/shared/dependency/analyzer/dependencyclasses/WarMainDependencyClassesProviderTest.java: ########## @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.shared.dependency.analyzer.dependencyclasses; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Collections; +import java.util.Set; +import java.util.stream.Stream; + +import org.apache.maven.model.Build; +import org.apache.maven.model.Plugin; +import org.apache.maven.project.MavenProject; +import org.apache.maven.shared.dependency.analyzer.ClassesPatterns; +import org.apache.maven.shared.dependency.analyzer.DependencyUsage; +import org.codehaus.plexus.util.xml.Xpp3Dom; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class WarMainDependencyClassesProviderTest { + + @Mock + private MavenProject project; + + private final WarMainDependencyClassesProvider provider = new WarMainDependencyClassesProvider(); + + @Test + void parseDefaultWebXml() throws IOException, URISyntaxException { + Path basePath = Paths.get(getClass().getResource("/webapp").toURI()); + when(project.getBasedir()).thenReturn(basePath.toFile()); + when(project.getPackaging()).thenReturn("war"); + + Set<DependencyUsage> classes = + provider.getDependencyClasses(project, new ClassesPatterns(Collections.singleton(".*\\.Servlet$"))); + + assertThat(classes) + .map(DependencyUsage::getDependencyClass) + .containsExactlyInAnyOrder("org.example.test.Filter", "org.example.test.Listener"); + } + + @Test + void noDefaultWebXml() throws IOException, URISyntaxException { + Path basePath = Paths.get(getClass().getResource("/webapp/examples").toURI()); + when(project.getBasedir()).thenReturn(basePath.toFile()); + when(project.getPackaging()).thenReturn("war"); + when(project.getBuild()).thenReturn(new Build()); + + Set<DependencyUsage> classes = provider.getDependencyClasses(project, new ClassesPatterns()); + + assertThat(classes).isEmpty(); + } + + public static Stream<Arguments> examplesData() { + return Stream.of( + Arguments.of("empty-web.xml", new String[] {}), + Arguments.of("nons-web.xml", new String[] { + "org.example.test.Filter", "org.example.test.Listener", "org.example.test.Servlet" + }), + Arguments.of("multi-web.xml", new String[] { + "org.example.test.Filter1", + "org.example.test.Filter2", + "org.example.test.Listener1", + "org.example.test.Listener2" + }), + Arguments.of("wrong-web.xml", new String[] {}), + Arguments.of("no-exists-web.xml", new String[] {})); + } + + @ParameterizedTest + @MethodSource("examplesData") + void examples(String webXmlName, String[] expectedCLasses) throws Exception { + setupProjectWithWebXml(webXmlName); + + Set<DependencyUsage> classes = provider.getDependencyClasses(project, new ClassesPatterns()); + + assertThat(classes).map(DependencyUsage::getDependencyClass).containsExactlyInAnyOrder(expectedCLasses); Review Comment: Parameter name has a typo: "expectedCLasses" should be "expectedClasses" (lowercase 'l' in 'Classes'). ```suggestion void examples(String webXmlName, String[] expectedClasses) throws Exception { setupProjectWithWebXml(webXmlName); Set<DependencyUsage> classes = provider.getDependencyClasses(project, new ClassesPatterns()); assertThat(classes).map(DependencyUsage::getDependencyClass).containsExactlyInAnyOrder(expectedClasses); ``` ########## src/it/web-application/pom.xml: ########## @@ -0,0 +1,73 @@ +<?xml version="1.0" encoding="UTF-8"?> + +<!-- + ~ Licensed to the Apache Software Foundation (ASF) under one + ~ or more contributor license agreements. See the NOTICE file + ~ distributed with this work for additional information + ~ regarding copyright ownership. The ASF licenses this file + ~ to you under the Apache License, Version 2.0 (the + ~ "License"); you may not use this file except in compliance + ~ with the License. You may obtain a copy of the License at + ~ + ~ http://www.apache.org/licenses/LICENSE-2.0 + ~ + ~ Unless required by applicable law or agreed to in writing, + ~ software distributed under the License is distributed on an + ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + ~ KIND, either express or implied. See the License for the + ~ specific language governing permissions and limitations + ~ under the License. + --> + +<project + xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <groupId>org.apache.maven.shared.dependency-analyzer.tests</groupId> + <artifactId>web-applicaiton</artifactId> Review Comment: Artifact ID has a typo: "web-applicaiton" should be "web-application" (missing 't' in 'application'). ```suggestion <artifactId>web-application</artifactId> ``` ########## src/it/web-application/web2/pom.xml: ########## @@ -0,0 +1,53 @@ +<?xml version="1.0" encoding="UTF-8"?> + +<!-- + ~ Licensed to the Apache Software Foundation (ASF) under one + ~ or more contributor license agreements. See the NOTICE file + ~ distributed with this work for additional information + ~ regarding copyright ownership. The ASF licenses this file + ~ to you under the Apache License, Version 2.0 (the + ~ "License"); you may not use this file except in compliance + ~ with the License. You may obtain a copy of the License at + ~ + ~ http://www.apache.org/licenses/LICENSE-2.0 + ~ + ~ Unless required by applicable law or agreed to in writing, + ~ software distributed under the License is distributed on an + ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + ~ KIND, either express or implied. See the License for the + ~ specific language governing permissions and limitations + ~ under the License. + --> + +<project + xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.maven.shared.dependency-analyzer.tests</groupId> + <artifactId>web-applicaiton</artifactId> Review Comment: Parent artifact ID has a typo: "web-applicaiton" should be "web-application" (missing 't' in 'application'). This must match the parent POM's artifactId. ```suggestion <artifactId>web-application</artifactId> ``` ########## src/main/java/org/apache/maven/shared/dependency/analyzer/dependencyclasses/WarMainDependencyClassesProvider.java: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.shared.dependency.analyzer.dependencyclasses; + +import javax.inject.Named; +import javax.inject.Singleton; +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +import java.io.File; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +import org.apache.maven.model.Plugin; +import org.apache.maven.project.MavenProject; +import org.apache.maven.shared.dependency.analyzer.ClassesPatterns; +import org.apache.maven.shared.dependency.analyzer.DependencyUsage; +import org.apache.maven.shared.dependency.analyzer.MainDependencyClassesProvider; +import org.codehaus.plexus.util.xml.Xpp3Dom; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; +import org.xml.sax.SAXException; + +/** + * Implementation of {@link MainDependencyClassesProvider} for web applications. + */ +@Named +@Singleton +class WarMainDependencyClassesProvider implements MainDependencyClassesProvider { + + private static final Logger LOGGER = LoggerFactory.getLogger(WarMainDependencyClassesProvider.class); + + @Override + public Set<DependencyUsage> getDependencyClasses(MavenProject project, ClassesPatterns excludedClasses) + throws IOException { + if (!"war".equals(project.getPackaging())) { + return Collections.emptySet(); + } + + File webXml = findWebXml(project); + if (webXml == null) { + LOGGER.debug("No web.xml found for project {}", project); + return Collections.emptySet(); + } + + if (!webXml.isFile()) { + LOGGER.debug("{} is not a file in project {}", webXml, project); + return Collections.emptySet(); + } + + try { + return processWebXml(webXml, excludedClasses); + } catch (SAXException | ParserConfigurationException e) { + LOGGER.warn("Error parsing web.xml file {}: {}", webXml, e.getMessage()); + return Collections.emptySet(); + } + } + + private File findWebXml(MavenProject project) { + // standard location + File webXmlFile = new File(project.getBasedir(), "src/main/webapp/WEB-INF/web.xml"); + if (webXmlFile.isFile()) { + return webXmlFile; + } + + // check maven-war-plugin configuration for custom location of web.xml + Plugin plugin = project.getBuild().getPluginsAsMap().get("org.apache.maven.plugins:maven-war-plugin"); + if (plugin == null) { + // should not happen as we are in a war project + LOGGER.debug("No war plugin found for project {}", project); + return null; + } + + return Optional.ofNullable(plugin.getConfiguration()) + .map(Xpp3Dom.class::cast) + .map(config -> config.getChild("webXml")) + .map(Xpp3Dom::getValue) + .map(path -> new File(project.getBasedir(), path)) + .orElse(null); + } + + private Set<DependencyUsage> processWebXml(File webXml, ClassesPatterns excludedClasses) + throws IOException, SAXException, ParserConfigurationException { + + DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); + documentBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + documentBuilderFactory.setNamespaceAware(true); + documentBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd";, false); + + DocumentBuilder documentBuilder = documentBuilderFactory.newDocumentBuilder(); + + Document doc = documentBuilder.parse(webXml); + + List<String> classes = new ArrayList<>(); + + processClassesFromTags(doc, classes, "filter-class"); + processClassesFromTags(doc, classes, "listener-class"); + processClassesFromTags(doc, classes, "servlet-class"); + + return classes.stream() + .filter(className -> !excludedClasses.isMatch(className)) + .map(className -> new DependencyUsage(className, webXml.toString())) + .collect(Collectors.toSet()); + } + + private void processClassesFromTags(Document doc, List<String> classes, String tagName) { + NodeList tags = doc.getElementsByTagName(tagName); + for (int i = 0; i < tags.getLength(); i++) { + Node node = tags.item(i); + classes.add(node.getTextContent()); Review Comment: The textContent from XML nodes includes leading and trailing whitespace. Consider using trim() on the class name to handle XML files with different formatting, where class names might be on separate lines with indentation. This would make the implementation more robust against various XML formatting styles. ```suggestion String className = node.getTextContent(); if (className != null) { className = className.trim(); if (!className.isEmpty()) { classes.add(className); } } ``` ########## src/it/web-application/web1/pom.xml: ########## @@ -0,0 +1,38 @@ +<?xml version="1.0" encoding="UTF-8"?> + +<!-- + ~ Licensed to the Apache Software Foundation (ASF) under one + ~ or more contributor license agreements. See the NOTICE file + ~ distributed with this work for additional information + ~ regarding copyright ownership. The ASF licenses this file + ~ to you under the Apache License, Version 2.0 (the + ~ "License"); you may not use this file except in compliance + ~ with the License. You may obtain a copy of the License at + ~ + ~ http://www.apache.org/licenses/LICENSE-2.0 + ~ + ~ Unless required by applicable law or agreed to in writing, + ~ software distributed under the License is distributed on an + ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + ~ KIND, either express or implied. See the License for the + ~ specific language governing permissions and limitations + ~ under the License. + --> + +<project + xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.maven.shared.dependency-analyzer.tests</groupId> + <artifactId>web-applicaiton</artifactId> Review Comment: Parent artifact ID has a typo: "web-applicaiton" should be "web-application" (missing 't' in 'application'). This must match the parent POM's artifactId. ```suggestion <artifactId>web-application</artifactId> ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
