Move API to autograder-api & dynamic loading of autograder-core and autograder-extra

[Feuermagier]

This PR implements dynamic loading of autograder-core and autograder-extra (and dependencies). This fixes #564, similar to #566, but with different tradeoffs. The main benefit is that this implementation relies less on fragile class loader tricks. However, changing an already loaded implementation from within a running process is not supported.

The PR introduces two new modules:

• autograder-api: Implements the core API to the Autograder, without a dependency on any other autograder module (nor spoon). Notably missing is UploadedFile, which is not required for the simple API exposed to downstream tools such as the Grading Tool. The core classes that users interact with (Linter, Problem, TempLocation, CodePosition) are converted to interfaces, and the old implementations are moved to XYImpl. autograder-core and autograder-extra always use these concrete implementations instead of the interfaces, since the interfaces only expose the functionality required for downstream use. Downstream users now have two options:
  • Depend on autograder-core (and optionally autograder-extra) and directly instantiate the respective Impls. The tests and autograder-cmd do this. There is no reflection or download of code involved in this approach.
  • Don't depend on autograder-core, but use the dynamic loading utilities in autograder-api. These utilities can download the autograder-full JAR from GitHub, load the classes into the running process, and discover the Impls using reflection.
• autograder-full: This module contains no code and is only used to build a JAR release that can be downloaded by the dynamic loader.

[Luro02]

I am quite unhappy that this PR renames the types to ProblemImpl/TempLocationImpl for the autograder-api interfaces. The interfaces are mostly for dynamic loading and are not used in the autograder-core/extra codebase.

My suggestion is to rename the interfaces in autograder-api and keep the original names for the Problem/TempLocation classes. That would make the number of changes a lot smaller and the code would look like before this PR.

Maybe ProblemFacade/TempLocationFacade?

[Feuermagier]

I am quite unhappy that this PR renames the types to ProblemImpl/TempLocationImpl for the autograder-api interfaces. The interfaces are mostly for dynamic loading and are not used in the autograder-core/extra codebase.

My suggestion is to rename the interfaces in autograder-api and keep the original names for the Problem/TempLocation classes. That would make the number of changes a lot smaller and the code would look like before this PR.

Maybe ProblemFacade/TempLocationFacade?

I've changed it to AbstractLinter, AbstractTempLocation, AbstractCodePosition, and AbstractProblem (which is a bit dumb, because the 'concrete' Problem is itself abstract, but at least it's consistent).

[Luro02]

Why is ProblemType in api? I guess the reason is for type-safety?

I wouldn't expose the enum to downstream, only via Strings for the enum constants. This should prevent problems with potential future changes and the code using autograder-api does not instantiate the ProblemType variants explicitly? It should only parse the config, maybe not even that. If type-safety is necessary, then expose a wrapper class like this:

public record AutograderProblemType(String name) {}

// in `ProblemType` one could add a method to safely convert between them:
public enum ProblemType {
    ...;

    public AutograderProblemType toAutograderProblemType() {
        return new AutograderProblemType(this.toString());
    }
}

The current implementation would crash when new releases introduce new checks because these are not loaded via the class loader. It would be exceedingly difficult to replace the ProblemType enum with a different implementation, like making it an interface and having different implementations for autograder-core and autograder-extra.

The current implementation doesn't provide a good error-message when there are unknown ProblemTypes. For the future we might want to parse the problem types individually and collect any unknown ones, before throwing an error. This could conflict with the current design as well.

[dfuchss]

Why is ProblemType in api? I guess the reason is for type-safety?

I wouldn't expose the enum to downstream, only via Strings for the enum constants.

This should prevent problems with potential future changes and the code using

autograder-api does not instantiate the ProblemType variants explicitly? It should

only parse the config, maybe not even that. If type-safety is necessary, then expose a wrapper class like this:

public record AutograderProblemType(String name) {}

// in `ProblemType` one could add a method to safely convert between them:

public enum ProblemType {

    ...;

    public AutograderProblemType toAutograderProblemType() {

        return new AutograderProblemType(this.toString());

    }

}

The current implementation would crash when new releases introduce new checks because these are not loaded via the class loader. It would be exceedingly difficult to replace the ProblemType enum with a different implementation, like making it an interface and having different implementations for autograder-core and autograder-extra.

The current implementation doesn't provide a good error-message when there are unknown ProblemTypes. For the future we might want to parse the problem types individually and collect any unknown ones, before throwing an error. This could conflict with the current design as well.

I'd agree. if it changes often it should moved from the API and replaced by some interface for deserialization, we can provide the interface-class-relationship to Jackson . If it's kind of stable , it could stay here . That's something you should decide :)

[Feuermagier]

Why is ProblemType in api? I guess the reason is for type-safety?

I wouldn't expose the enum to downstream, only via Strings for the enum constants. This should prevent problems with potential future changes and the code using autograder-api does not instantiate the ProblemType variants explicitly? It should only parse the config, maybe not even that. If type-safety is necessary, then expose a wrapper class like this:

public record AutograderProblemType(String name) {}

// in `ProblemType` one could add a method to safely convert between them:
public enum ProblemType {
    ...;

    public AutograderProblemType toAutograderProblemType() {
        return new AutograderProblemType(this.toString());
    }
}

The current implementation would crash when new releases introduce new checks because these are not loaded via the class loader. It would be exceedingly difficult to replace the ProblemType enum with a different implementation, like making it an interface and having different implementations for autograder-core and autograder-extra.

The current implementation doesn't provide a good error-message when there are unknown ProblemTypes. For the future we might want to parse the problem types individually and collect any unknown ones, before throwing an error. This could conflict with the current design as well.

The current config format isn't going to be used with the new Artemis4J anyway, instead, the grading config and the autograder config will be merged (like so).

My intention with moving ProblemType to autograder-api was indeed type safety, though I see that this makes adding checks a lot harder, so I'm fine with using Strings only (@dfuchss also proposed this).

[dfuchss]

Why is ProblemType in api? I guess the reason is for type-safety?

I wouldn't expose the enum to downstream, only via Strings for the enum constants. This should prevent problems with potential future changes and the code using autograder-api does not instantiate the ProblemType variants explicitly? It should only parse the config, maybe not even that. If type-safety is necessary, then expose a wrapper class like this:

public record AutograderProblemType(String name) {}

// in `ProblemType` one could add a method to safely convert between them:

public enum ProblemType {

    ...;

    public AutograderProblemType toAutograderProblemType() {

        return new AutograderProblemType(this.toString());

    }

}

The current implementation would crash when new releases introduce new checks because these are not loaded via the class loader. It would be exceedingly difficult to replace the ProblemType enum with a different implementation, like making it an interface and having different implementations for autograder-core and autograder-extra.

The current implementation doesn't provide a good error-message when there are unknown ProblemTypes. For the future we might want to parse the problem types individually and collect any unknown ones, before throwing an error. This could conflict with the current design as well.

The current config format isn't going to be used with the new Artemis4J anyway, instead, the grading config and the autograder config will be merged (like so).

My intention with moving ProblemType to autograder-api was indeed type safety, though I see that this makes adding checks a lot harder, so I'm fine with using Strings only (@dfuchss also proposed this).

Just to make it clear: I really like the idea of having this enum in the API ; esp. because of TypeSafety in the Config Class. Nevertheless, you do know more about the frequency of changes to this enum :)

[Feuermagier]

It changes quite often (e.g. when we discover a frequent, easy to detect mistake during grading, and want to implement a new check for that). I'm going to use Strings instead of a wrapper class so that Artemis4J's config format is not going to depend on the presence of Jackson annotations within autograder-api

[dfuchss]

Why is ProblemType in api? I guess the reason is for type-safety?

I wouldn't expose the enum to downstream, only via Strings for the enum constants. This should prevent problems with potential future changes and the code using autograder-api does not instantiate the ProblemType variants explicitly? It should only parse the config, maybe not even that. If type-safety is necessary, then expose a wrapper class like this:

public record AutograderProblemType(String name) {}

// in `ProblemType` one could add a method to safely convert between them:

public enum ProblemType {

    ...;

    public AutograderProblemType toAutograderProblemType() {

        return new AutograderProblemType(this.toString());

    }

}

The current implementation would crash when new releases introduce new checks because these are not loaded via the class loader. It would be exceedingly difficult to replace the ProblemType enum with a different implementation, like making it an interface and having different implementations for autograder-core and autograder-extra.

The current implementation doesn't provide a good error-message when there are unknown ProblemTypes. For the future we might want to parse the problem types individually and collect any unknown ones, before throwing an error. This could conflict with the current design as well.

The current config format isn't going to be used with the new Artemis4J anyway, instead, the grading config and the autograder config will be merged (like so).

My intention with moving ProblemType to autograder-api was indeed type safety, though I see that this makes adding checks a lot harder, so I'm fine with using Strings only (@dfuchss also proposed this).

Just to make it clear: I really like the idea of having this enum in the API ; esp. because of TypeSafety in the Config Class. Nevertheless, you do know more about the frequency of changes to this enum :)

I'd also would define a interface that is implemented by the enum instead of strings if it has to be moved. Thereby, the config can use the interface and the only thing to do is to provide Jackson with the information which enum class implements the interface

[Feuermagier]

How do you imagine the Jackson thing, given that the Enum is not available at compile time so it can't be used within annotations? Via a custom deserializer?

[Luro02]

Should we really have jackson in the interface? Couldn't we have something like this instead?:

interface AbstractProblemType {
    AbstractProblemType fromString(String string);

    List<AbstractProblemType> fromStrings(List<String> strings);
}

[Feuermagier]

Looks like a good idea to me

Edit: It's sadly not so simple, since fromString should be static, so the method needs to be called via reflection

[dfuchss]

How do you imagine the Jackson thing, given that the Enum is not available at compile time so it can't be used within annotations? Via a custom deserializer?

Just define a simpleModule and define Mapping from Interface to ImplClass :) That should work. https://fasterxml.github.io/jackson-databind/javadoc/2.7/com/fasterxml/jackson/databind/module/SimpleModule.html#addAbstractTypeMapping(java.lang.Class,%20java.lang.Class)

But the current solution is also fine. It simply depends on who is responsible for deserialization: Jackson or you :)