gnodet-bot commented on code in PR #13094: URL: https://github.com/apache/maven/pull/13094#discussion_r4273028197
########## impl/maven-core/src/main/java/org/apache/maven/resolver/SpiWorkspaceReaderAdapter.java: ########## @@ -0,0 +1,213 @@ +/* + * 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.resolver; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; + +import javax.xml.stream.XMLStreamException; + +import org.apache.maven.api.ArtifactCoordinates; +import org.apache.maven.api.Version; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.model.Model; +import org.apache.maven.impl.resolver.MavenWorkspaceReader; +import org.apache.maven.model.v4.MavenStaxReader; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.repository.WorkspaceRepository; + +/** + * Bridge adapter that wraps an {@link org.apache.maven.api.spi.WorkspaceReader SPI WorkspaceReader} + * into a {@link MavenWorkspaceReader}. + * + * <p>This adapter translates between resolver {@link Artifact} and Maven API + * {@link org.apache.maven.api.Artifact} types, allowing SPI implementations to work + * with Maven 4 API types exclusively while being integrated into the resolver's + * workspace reader chain. By implementing {@link MavenWorkspaceReader}, the adapter + * also participates in model resolution: {@link #findModel(Artifact)} delegates to + * {@link org.apache.maven.api.spi.WorkspaceReader#findModel(org.apache.maven.api.Artifact)} + * and parses the returned POM path into a v4 {@link Model}. + * + * @since 4.1.0 + */ +public class SpiWorkspaceReaderAdapter implements MavenWorkspaceReader { + + private final org.apache.maven.api.spi.WorkspaceReader delegate; + private final WorkspaceRepository repository; + + public SpiWorkspaceReaderAdapter(org.apache.maven.api.spi.WorkspaceReader delegate) { + this.delegate = delegate; + this.repository = new WorkspaceRepository("spi-" + delegate.getClass().getSimpleName()); + } + + @Override + public WorkspaceRepository getRepository() { + return repository; + } + + @Override + public File findArtifact(Artifact artifact) { + Optional<Path> result = delegate.findArtifact(toApiArtifact(artifact)); + return result.map(Path::toFile).orElse(null); + } + + @Override + public List<String> findVersions(Artifact artifact) { + return delegate.findVersions(toApiArtifact(artifact)); + } + + @Override + public Model findModel(Artifact artifact) { + Optional<Path> modelPath = delegate.findModel(toApiArtifact(artifact)); + if (modelPath.isEmpty()) { + return null; + } + try (InputStream is = Files.newInputStream(modelPath.get())) { + return new MavenStaxReader().read(is, false, null); + } catch (IOException | XMLStreamException e) { + // Return null — consistent with other MavenWorkspaceReader implementations. + // The caller (DefaultArtifactDescriptorReader) will fall back to normal resolution. + return null; Review Comment: ⚠️ **Silent exception swallowing:** When `MavenStaxReader` fails to parse the POM, the exception is caught and `null` is returned without any log entry. The caller (`DefaultArtifactDescriptorReader`) will fall back to normal resolution — but the IDE implementor has no way to diagnose why their `findModel()` return value was ignored. Add at least a debug-level log: ```suggestion } catch (IOException | XMLStreamException e) { // Fall back to normal resolution — log at debug so integrators can diagnose. if (LOGGER.isDebugEnabled()) { LOGGER.debug("Failed to parse model from workspace reader for {}: {}", artifact, e.getMessage()); } return null; } ``` (requires adding `private static final Logger LOGGER = LoggerFactory.getLogger(SpiWorkspaceReaderAdapter.class);`) ########## impl/maven-core/src/main/java/org/apache/maven/resolver/SpiWorkspaceReaderAdapter.java: ########## @@ -0,0 +1,213 @@ +/* + * 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.resolver; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; + +import javax.xml.stream.XMLStreamException; + +import org.apache.maven.api.ArtifactCoordinates; +import org.apache.maven.api.Version; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.model.Model; +import org.apache.maven.impl.resolver.MavenWorkspaceReader; +import org.apache.maven.model.v4.MavenStaxReader; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.repository.WorkspaceRepository; + +/** + * Bridge adapter that wraps an {@link org.apache.maven.api.spi.WorkspaceReader SPI WorkspaceReader} + * into a {@link MavenWorkspaceReader}. + * + * <p>This adapter translates between resolver {@link Artifact} and Maven API + * {@link org.apache.maven.api.Artifact} types, allowing SPI implementations to work + * with Maven 4 API types exclusively while being integrated into the resolver's + * workspace reader chain. By implementing {@link MavenWorkspaceReader}, the adapter + * also participates in model resolution: {@link #findModel(Artifact)} delegates to + * {@link org.apache.maven.api.spi.WorkspaceReader#findModel(org.apache.maven.api.Artifact)} + * and parses the returned POM path into a v4 {@link Model}. + * + * @since 4.1.0 + */ +public class SpiWorkspaceReaderAdapter implements MavenWorkspaceReader { + + private final org.apache.maven.api.spi.WorkspaceReader delegate; + private final WorkspaceRepository repository; + + public SpiWorkspaceReaderAdapter(org.apache.maven.api.spi.WorkspaceReader delegate) { + this.delegate = delegate; + this.repository = new WorkspaceRepository("spi-" + delegate.getClass().getSimpleName()); + } + + @Override + public WorkspaceRepository getRepository() { + return repository; + } + + @Override + public File findArtifact(Artifact artifact) { + Optional<Path> result = delegate.findArtifact(toApiArtifact(artifact)); + return result.map(Path::toFile).orElse(null); + } + + @Override + public List<String> findVersions(Artifact artifact) { + return delegate.findVersions(toApiArtifact(artifact)); + } + + @Override + public Model findModel(Artifact artifact) { + Optional<Path> modelPath = delegate.findModel(toApiArtifact(artifact)); + if (modelPath.isEmpty()) { + return null; + } + try (InputStream is = Files.newInputStream(modelPath.get())) { + return new MavenStaxReader().read(is, false, null); + } catch (IOException | XMLStreamException e) { + // Return null — consistent with other MavenWorkspaceReader implementations. + // The caller (DefaultArtifactDescriptorReader) will fall back to normal resolution. + return null; + } + } + + /** + * Returns the underlying SPI workspace reader. + */ + public org.apache.maven.api.spi.WorkspaceReader getDelegate() { + return delegate; + } + + /** + * Creates a lightweight Maven API {@link org.apache.maven.api.Artifact} from a resolver artifact + * without requiring an active session. + */ + private static org.apache.maven.api.Artifact toApiArtifact(Artifact artifact) { + return new LightweightApiArtifact(artifact); + } + + /** + * A lightweight implementation of {@link org.apache.maven.api.Artifact} that wraps a resolver artifact + * for the purpose of passing artifact coordinates to SPI workspace readers. + */ + private static class LightweightApiArtifact implements org.apache.maven.api.Artifact { + private final Artifact artifact; + private final String key; + + LightweightApiArtifact(Artifact artifact) { + this.artifact = artifact; + this.key = getGroupId() + + ':' + + getArtifactId() + + ':' + + getExtension() + + (getClassifier().isEmpty() ? "" : ":" + getClassifier()) + + ':' + + artifact.getVersion(); + } Review Comment: ⚠️ **Rebase hazard — missing noun-style accessors:** `master` has completed the `Artifact` API migration: `groupId()`, `artifactId()`, `version()`, `baseVersion()`, `classifier()`, and `extension()` are now the abstract contract; `getGroupId()` etc. are `default` shims that delegate to them. This class only overrides the deprecated `getX()` variants but never implements `groupId()`, `artifactId()`, etc. After rebasing onto `master`, the Java compiler will reject this class as abstract (6 unimplemented abstract methods). Fix: implement the noun-style methods and remove (or keep for backward compat) the `getX()` overrides: ```suggestion LightweightApiArtifact(Artifact artifact) { this.artifact = artifact; this.key = groupId() + ':' + artifactId() + ':' + extension() + (classifier().isEmpty() ? "" : ":" + classifier()) + ':' + artifact.getVersion(); } ``` ########## impl/maven-core/src/main/java/org/apache/maven/resolver/SpiWorkspaceReaderAdapter.java: ########## @@ -0,0 +1,213 @@ +/* + * 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.resolver; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; + +import javax.xml.stream.XMLStreamException; + +import org.apache.maven.api.ArtifactCoordinates; +import org.apache.maven.api.Version; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.model.Model; +import org.apache.maven.impl.resolver.MavenWorkspaceReader; +import org.apache.maven.model.v4.MavenStaxReader; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.repository.WorkspaceRepository; + +/** + * Bridge adapter that wraps an {@link org.apache.maven.api.spi.WorkspaceReader SPI WorkspaceReader} + * into a {@link MavenWorkspaceReader}. + * + * <p>This adapter translates between resolver {@link Artifact} and Maven API + * {@link org.apache.maven.api.Artifact} types, allowing SPI implementations to work + * with Maven 4 API types exclusively while being integrated into the resolver's + * workspace reader chain. By implementing {@link MavenWorkspaceReader}, the adapter + * also participates in model resolution: {@link #findModel(Artifact)} delegates to + * {@link org.apache.maven.api.spi.WorkspaceReader#findModel(org.apache.maven.api.Artifact)} + * and parses the returned POM path into a v4 {@link Model}. + * + * @since 4.1.0 + */ +public class SpiWorkspaceReaderAdapter implements MavenWorkspaceReader { + + private final org.apache.maven.api.spi.WorkspaceReader delegate; + private final WorkspaceRepository repository; + + public SpiWorkspaceReaderAdapter(org.apache.maven.api.spi.WorkspaceReader delegate) { + this.delegate = delegate; + this.repository = new WorkspaceRepository("spi-" + delegate.getClass().getSimpleName()); + } + + @Override + public WorkspaceRepository getRepository() { + return repository; + } + + @Override + public File findArtifact(Artifact artifact) { + Optional<Path> result = delegate.findArtifact(toApiArtifact(artifact)); + return result.map(Path::toFile).orElse(null); + } + + @Override + public List<String> findVersions(Artifact artifact) { + return delegate.findVersions(toApiArtifact(artifact)); + } + + @Override + public Model findModel(Artifact artifact) { + Optional<Path> modelPath = delegate.findModel(toApiArtifact(artifact)); + if (modelPath.isEmpty()) { + return null; + } + try (InputStream is = Files.newInputStream(modelPath.get())) { + return new MavenStaxReader().read(is, false, null); + } catch (IOException | XMLStreamException e) { + // Return null — consistent with other MavenWorkspaceReader implementations. + // The caller (DefaultArtifactDescriptorReader) will fall back to normal resolution. + return null; + } + } + + /** + * Returns the underlying SPI workspace reader. + */ + public org.apache.maven.api.spi.WorkspaceReader getDelegate() { + return delegate; + } + + /** + * Creates a lightweight Maven API {@link org.apache.maven.api.Artifact} from a resolver artifact + * without requiring an active session. + */ + private static org.apache.maven.api.Artifact toApiArtifact(Artifact artifact) { + return new LightweightApiArtifact(artifact); + } + + /** + * A lightweight implementation of {@link org.apache.maven.api.Artifact} that wraps a resolver artifact + * for the purpose of passing artifact coordinates to SPI workspace readers. + */ + private static class LightweightApiArtifact implements org.apache.maven.api.Artifact { + private final Artifact artifact; + private final String key; + + LightweightApiArtifact(Artifact artifact) { + this.artifact = artifact; + this.key = getGroupId() + + ':' + + getArtifactId() + + ':' + + getExtension() + + (getClassifier().isEmpty() ? "" : ":" + getClassifier()) + + ':' + + artifact.getVersion(); + } + + @Override + public String key() { + return key; + } + + @Nonnull + @Override + public String getGroupId() { Review Comment: ⚠️ **Missing noun-style accessor implementations (see rebase hazard above).** The class currently overrides the deprecated `getGroupId()` / `getArtifactId()` / `getVersion()` / `getBaseVersion()` / `getExtension()` / `getClassifier()` accessors, but `master` declares `groupId()`, `artifactId()`, `version()`, `baseVersion()`, `classifier()`, `extension()` as abstract. These 6 methods need to be implemented: ```java @Nonnull @Override public String groupId() { return artifact.getGroupId(); } @Nonnull @Override public String artifactId() { return artifact.getArtifactId(); } // … similarly for version(), baseVersion(), classifier(), extension() ``` ########## impl/maven-core/src/main/java/org/apache/maven/DefaultMaven.java: ########## @@ -348,8 +349,15 @@ private void setupWorkspaceReader(MavenSession session, MavenChainedWorkspaceRea workspaceReaders.add(repoWorkspaceReader); } } - // 3) .. n) Project-scoped workspace readers + // 3) .. n) Project-scoped legacy workspace readers (org.eclipse.aether) workspaceReaders.addAll(getProjectScopedExtensionComponents(session.getProjects(), WorkspaceReader.class)); + // n+1) SPI workspace readers (org.apache.maven.api.spi) — discovered here, after buildGraph() + // has loaded core extensions into the container so they are visible via + // getProjectScopedExtensionComponents. Review Comment: 🔧 **Nit — comment continuation indent dropped on third line:** ``` // n+1) SPI workspace readers (org.apache.maven.api.spi) — discovered here, after buildGraph() // has loaded core extensions into the container so they are visible via // getProjectScopedExtensionComponents. ← should be indented to align with the line above ``` ```suggestion // getProjectScopedExtensionComponents. ``` -- 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]
