gnodet-bot commented on code in PR #13729: URL: https://github.com/apache/maven/pull/13729#discussion_r4392036936
########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/WorkspaceReader.java: ########## @@ -0,0 +1,63 @@ +/* + * 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.api.spi; + +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; + +import org.apache.maven.api.Artifact; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; + +/** + * WorkspaceReader SPI contract. + * + * @since 4.0.0 + */ +@Experimental +public interface WorkspaceReader { Review Comment: ⚠️ **Missing SPI conventions** — all SPI interfaces in `maven-api-spi` extend `SpiService`, carry `@Consumer` and `@Named`, and are discovered via DI. This interface is missing all three, which means implementations cannot be registered via Maven's extension mechanism and will be silently ignored at runtime. Compare with `ModelParser`, `ModelTransformer`, `LanguageProvider`, etc. — they all follow this pattern. ```suggestion @Experimental @Consumer @Named public interface WorkspaceReader extends SpiService { ``` You'll also need to add these imports: ```java import org.apache.maven.api.annotations.Consumer; import org.apache.maven.api.di.Named; ``` ########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/WorkspaceReader.java: ########## @@ -0,0 +1,63 @@ +/* + * 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.api.spi; + +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; + +import org.apache.maven.api.Artifact; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; + +/** + * WorkspaceReader SPI contract. + * + * @since 4.0.0 Review Comment: ⚠️ **Wrong `@since` version** — `WorkspaceReader` does not exist in Maven 4.0.0 (released). This is a new SPI being added to master (4.1.0-SNAPSHOT). Using `@since 4.0.0` would incorrectly document it as available in a version where it doesn't exist. ```suggestion * @since 4.1.0 ``` ########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/WorkspaceReader.java: ########## @@ -0,0 +1,63 @@ +/* + * 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.api.spi; + +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; + +import org.apache.maven.api.Artifact; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; + +/** + * WorkspaceReader SPI contract. + * + * @since 4.0.0 + */ +@Experimental +public interface WorkspaceReader { + + /** + * Finds the artifact in the workspace. + * + * @param artifact the artifact to find, must not be {@code null} + * @return the path to the artifact, or {@code null} if not found + */ + Path findArtifact(@Nonnull Artifact artifact); Review Comment: ⚠️ **Nullable return breaks the `Optional`-first convention** — `maven-api-spi` uses `Optional<Path>` for "may not find" semantics (see `ModelParser.locate()`). Returning a raw nullable `Path` forces callers to null-check, which the Maven 4 API explicitly avoids. The Javadoc even admits this: `"or {@code null} if not found"` — a design smell for a new API. PR #13094 (the conflicting PR defining the same interface) correctly uses `Optional<Path>` here. ```suggestion * @return the path to the artifact, or empty if not found in workspace */ Optional<Path> findArtifact(@Nonnull Artifact artifact); ``` ########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/WorkspaceReader.java: ########## @@ -0,0 +1,63 @@ +/* + * 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.api.spi; + +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; + +import org.apache.maven.api.Artifact; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; + +/** + * WorkspaceReader SPI contract. + * + * @since 4.0.0 + */ +@Experimental +public interface WorkspaceReader { + + /** + * Finds the artifact in the workspace. + * + * @param artifact the artifact to find, must not be {@code null} + * @return the path to the artifact, or {@code null} if not found + */ + Path findArtifact(@Nonnull Artifact artifact); + + /** + * Finds the available versions for the artifact in the workspace. + * + * @param artifact the artifact to find versions for, must not be {@code null} + * @return the list of available versions, never {@code null} + */ + @Nonnull + List<String> findVersions(@Nonnull Artifact artifact); + + /** + * Finds the model for the given artifact, if available. + * + * @param artifact the artifact to find the model for, must not be {@code null} + * @return an optional path to the model, or empty if not found + */ + default Optional<Path> findModel(@Nonnull Artifact artifact) { Review Comment: ⚠️ **Wrong return type for `findModel`** — returning `Optional<Path>` (a path to a POM file) defeats the stated purpose of this method. The issue (#13515) and the PR description both say the goal is to allow extensions to *supply the model associated with an artifact, including cases where the POM is generated dynamically* (e.g. Tycho generating synthetic POMs for P2 artifacts). If you return a `Path`, Maven must still parse the POM from disk — that adds a filesystem roundtrip and forces the extension to write an intermediate XML file, which is exactly what the feature is trying to avoid. The correct return type is `Optional<Model>` (from `org.apache.maven.api.model.Model`), as implemented in PR #13094. This allows extensions to return an in-memory model bypassing `ModelProcessor` entirely. ```suggestion * @return the model for the artifact, or empty if not available in the workspace */ default Optional<Model> findModel(@Nonnull Artifact artifact) { ``` Add import: `import org.apache.maven.api.model.Model;` -- 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]
