Feature/474 load GitHub references #548

ysrbo · 2024-03-19T08:29:14Z

Fixes #474

...aspect-model-resolver/src/main/java/org/eclipse/esmf/aspectmodel/resolver/GroupStrategy.java

pom.xml

tools/esmf-aspect-model-github-resolver/pom.xml

tools/samm-cli/src/main/java/org/eclipse/esmf/aspect/AspectMigrateCommand.java

tools/samm-cli/src/test/java/org/eclipse/esmf/SammCliTest.java

…trategy input. eclipse-esmf#474

eclipse-esmf#474

atextor

The PR is missing two things:

It seems the GitHub Resolver is not activated in the Maven Plugin (I only see one invocation of its install() method, which is in samm-cli). It should be usable there and its functionality also tested, at least on a basic level. I expect that a new property needs to be added to configure the GitHub models root.
It is completely missing user documentation. I expect changes:
1. in java-aspect-tooling.adoc which explains the plugin loading mechanism in the AspectModelResolver
2. in samm-cli.adoc which explains which URIs can be used as INPUT (e.g. only file: URIs and URLs which point to GitHub, but not other Git hosts and not SAMM URNs)
3. in maven-plugin.adoc which explains that/how the GitHub resolver is used from the Maven Plugin.

atextor · 2024-03-28T07:42:07Z

...java/org/eclipse/esmf/aspectmodel/resolver/services/SammAspectMetaModelResourceResolver.java

-      return addToModel( model, resolver.apply( version ).stream().map( TurtleLoader::openUrl ) );
+      final Model result = ModelFactory.createDefaultModel();
+
+      var error = resolver.apply( version ).stream()


This variable can be final

atextor · 2024-03-28T07:43:24Z

...java/org/eclipse/esmf/aspectmodel/resolver/services/SammAspectMetaModelResourceResolver.java

+      var error = resolver.apply( version ).stream()
+            .map( url -> Try.withResources( url::openStream )
+                  .of( TurtleLoader::loadTurtle )
+                  .mapTry(Try::get)


Please apply the project code style; see the conventions on options for automatic checks.

atextor · 2024-04-08T08:24:24Z

...-model-resolver/src/main/java/org/eclipse/esmf/aspectmodel/resolver/AspectModelResolver.java

-         final Stream<String> subjectUri = statement.getSubject().isURIResource()
-               ? Stream.of( statement.getSubject().getURI() )
-               : Stream.empty();
+         final Stream<String> subjectUri = statement.getSubject().isURIResource() ?
+               Stream.of( statement.getSubject().getURI() ) : Stream.empty();
         final Stream<String> propertyUri = Stream.of( statement.getPredicate().getURI() );
-         final Stream<String> objectUri = statement.getObject().isURIResource()
-               ? Stream.of( statement.getObject().asResource().getURI() )
-               : Stream.empty();
+         final Stream<String> objectUri = statement.getObject().isURIResource() ?
+               Stream.of( statement.getObject().asResource().getURI() ) : Stream.empty();


Revert this formatting, the ?: operators should be at the beginning of the line

atextor · 2024-04-08T09:43:31Z

...-model-resolver/src/main/java/org/eclipse/esmf/aspectmodel/resolver/AspectModelResolver.java

    * @return the resolved model on success
    */
   public Try<VersionedModel> resolveAspectModel( final ResolutionStrategy resolutionStrategy, final AspectModelUrn input ) {
      return resolveAspectModel( resolutionStrategy, List.of( input ) );
   }

+   public Try<VersionedModel> resolveAspectModel( final ResolutionStrategy resolutionStrategy, final URI input ) {


In my opinion we should not add this method to the resolver API. Aspect Models may contain only resources that use valid Aspect Model URNs as their URIs. While they can also refer to other URIs (e.g., the XSD types), these do not need to be resolved, i.e., they never point to anything that needs resolving. Having a resolution that takes an arbitrary URI as input however suggests otherwise.

atextor · 2024-04-08T09:53:25Z

...-model-resolver/src/main/java/org/eclipse/esmf/aspectmodel/resolver/AspectModelResolver.java

 */
 public class AspectModelResolver {
   private static final Logger LOG = LoggerFactory.getLogger( AspectModelResolver.class );
+   private static final List<Pair<Predicate<URI>, Function<URI, Try<ResolutionStrategy>>>> STRATEGY_BUILDERS = new ArrayList<>();


Having "default" resolvers here is a good idea. I'd prefer to have the types more specific though, since we do not resolve arbitrary URIs but only AspectModelUrns (also see my other comment below). The ResolutionStrategy is already function that takes the respective Aspect Model URN as input for resolution, I would not wrap this in another function that decides whether the resolver can resolve a given URI; it's the duty of the resolver to decide this. So I'd propose this:

Instead of the List<Pair>, make ResolutionStrategy also implement Predicate<AspectModelUrn> so that it can decide and report if it can resolve a URN

Change this List accordingly to a List<ResolutionStrategy>

After having read more of the PR, I think the URI here is not supposed to identify the model element to resolve (i.e. its AspectModelUrn) but is already the source for the model (URL or file: URI)? In this case, I don't understand the setup at all; since this information is supposed to be known only to the ResolutionStrategy; the AspectModelResolver's task is only to execute the ResolutionStrategies and collect and merge their results. Where a model "comes from" should be encapsulated by the ResolutionStrategy.

atextor · 2024-04-09T04:39:07Z

...src/test/java/org/eclipse/esmf/aspectmodel/resolver/github/GitHubResolutionStrategyTest.java

+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.Mockito.*;


Don't use wildcard imports

atextor · 2024-04-09T04:41:12Z

...src/test/java/org/eclipse/esmf/aspectmodel/resolver/github/GitHubResolutionStrategyTest.java

+   void setUp() throws IOException {
+      testUrl = mock( URL.class );
+      when( testUrl.getHost() ).thenReturn( "github.com" );
+      //      when( testUrl.toURI() ).thenReturn( URI.create( GITHUB_URL + GITHUB_FILE )


Can this be removed?

atextor · 2024-04-09T04:42:17Z

tools/samm-cli/src/main/java/org/eclipse/esmf/AbstractCommand.java

@@ -13,20 +13,23 @@

 package org.eclipse.esmf;

-import static org.eclipse.esmf.aspectmodel.resolver.AspectModelResolver.fileToUrn;
+import static org.apache.commons.lang3.StringUtils.isBlank;
+import static org.eclipse.esmf.aspectmodel.resolver.AspectModelResolver.*;


Don't use wildcard imports

atextor · 2024-04-09T04:45:38Z

tools/samm-cli/src/main/java/org/eclipse/esmf/AbstractCommand.java

+            ? new File( uri )
+            : isBlank( uri.getScheme() )
+               ? new File( uri.getPath() )
+               : null;


Methods should never return null (with very few exceptions, e.g., if you your method returns Try<Void>, you can return Try.of(null)). If the value could be missing, return Optional<File> instead. Mapping URI to File is a partial function, i.e., not for every input value a corresponding output value can exists.

atextor · 2024-04-09T05:02:42Z

tools/samm-cli/src/test/java/org/eclipse/esmf/SammCliTest.java

@@ -913,7 +913,7 @@ private String resolverCommand() {
                     ? ".bat" : ".sh" ) ).getCanonicalPath();
         final String modelsRoot = new File( System.getProperty( "user.dir" ) + "/target/classes/valid" ).getCanonicalPath();
         final String metaModelVersion = KnownVersion.getLatest().toString().toLowerCase();
-         return resolverScript + " " + modelsRoot + " " + metaModelVersion;
+         return resolverScript + " " + modelsRoot + " " + metaModelVersion + " ";


Why is there a separate space at the end of the resolver command?

Yauhenikapl · 2024-08-19T11:46:12Z

Was implemented here: #608

ysrbo force-pushed the feature/474-load-github-references branch 5 times, most recently from 471d665 to 8238d21 Compare March 20, 2024 06:55

Yauhenikapl marked this pull request as ready for review March 20, 2024 09:03

Yauhenikapl requested review from Yauhenikapl and atextor March 20, 2024 09:04

Yauhenikapl reviewed Mar 20, 2024

View reviewed changes

ysrbo force-pushed the feature/474-load-github-references branch 4 times, most recently from 547c8fd to b0668e0 Compare March 21, 2024 06:52

Yauhenikapl approved these changes Mar 22, 2024

View reviewed changes

ysrbo force-pushed the feature/474-load-github-references branch 2 times, most recently from 33cee0a to 634505a Compare March 28, 2024 08:05

ysrbo added 2 commits March 28, 2024 09:05

Changed the file reference format to URI reference for the resolver s…

7d37c9a

…trategy input. eclipse-esmf#474

Added GitHub ResolutionStrategy.

3f924a9

eclipse-esmf#474

ysrbo force-pushed the feature/474-load-github-references branch from 634505a to 3f924a9 Compare March 28, 2024 08:05

atextor requested changes Apr 9, 2024

View reviewed changes

Yauhenikapl closed this Aug 19, 2024

atextor deleted the feature/474-load-github-references branch August 21, 2024 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/474 load GitHub references #548

Feature/474 load GitHub references #548

ysrbo commented Mar 19, 2024 •

edited by atextor

Loading

atextor left a comment

atextor Mar 28, 2024

atextor Mar 28, 2024

atextor Apr 8, 2024

atextor Apr 8, 2024

atextor Apr 8, 2024

atextor Apr 9, 2024

atextor Apr 9, 2024

atextor Apr 9, 2024

atextor Apr 9, 2024

atextor Apr 9, 2024

atextor Apr 9, 2024

Yauhenikapl commented Aug 19, 2024

		import static org.junit.jupiter.api.Assertions.*;
		import static org.mockito.Mockito.*;

Feature/474 load GitHub references #548

Feature/474 load GitHub references #548

Conversation

ysrbo commented Mar 19, 2024 • edited by atextor Loading

atextor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yauhenikapl commented Aug 19, 2024

ysrbo commented Mar 19, 2024 •

edited by atextor

Loading