Remove loadConfiguration where it is possible

[palagdan]

Resolves #274.

I removed loadConfiguration() where possible in s-pipes-modules. However, there are three main reasons why I couldn't remove it from some other modules:

• 1. Field dependency on runtime values: Some fields depend on values retrieved from previous fields, which is why these values can only be defined at runtime.

 @Override
   public void loadConfiguration() {
       e5xResourceUriStr = getEffectiveValue(KBSS_MODULE.JENA.has_resource_uri).asLiteral().toString();
       e5xResource = getResourceByUri(e5xResourceUriStr);
   }

• 2. Dependency on parent module configuration: Some modules invoke loadConfiguration() from parent modules.

   @Override
    public void loadConfiguration() {
        super.loadConfiguration();
        marginalConstraint = getEffectiveStringValue(TYPE_PREFIX + "marginal-constraint");
        marginalsDefsFileUrl = getEffectiveStringValue(TYPE_PREFIX + "marginals-defs-file-url");
        marginalsFileUrl = getEffectiveStringValue(TYPE_PREFIX + "marginals-file-url");
        dataServiceUrl = getEffectiveStringValue(TYPE_PREFIX + "data-service-url");
    }

• 3. Conditional field initialization: Some fields with parameters need to be initialized only under certain conditions.

    @Override
    public void loadConfiguration() {

        if (this.resource.getProperty(DescriptorModel.JENA.has_document_date) != null) { // TODO set current date if not specified
            documentDate = getEffectiveValue(DescriptorModel.JENA.has_document_date).asLiteral().toString();
        }

        if (this.resource.getProperty(DescriptorModel.JENA.has_rule_file) != null) { //TODO support more rule files
            ruleFilePaths.add(Paths.get(getEffectiveValue(DescriptorModel.JENA.has_rule_file).asLiteral().toString()));
        }
    }

[blcham]

• It seems like i) and iii) are not are not issue.
• iii) should be solved by calling function to load configuration after @parameter annotations are processed

[palagdan]

@blcham

I have removed all loadConfiguration() methods from the modules and replaced them with loadManualConfiguration(). Also I removed manual configuration for fields that can be configured using annotations.

[blcham]

See my comments.

[palagdan]

@blcham
I believe we can easily replace Integer with int because it is only used in one place, as follows:

module.setPageSize(10);

[palagdan]

@blcham

I’m not sure why, but when I try to test locally with mvn clean install, I get the same error that we solved in a previous ticket. However, all the tests pass in the GitHub environment. Do you have any suggestions on how I can fix this?

One more thing I wanted to discuss: how can I easily test all the modules that I modified in the current ticket? I want to be sure that I didn’t break anything and that everything works as expected. Some modules don’t have tests at all. Maybe it would be a good idea to implement tests for these modules in a separate ticket before I continue with the current ticket?

[palagdan]

@blcham

Also, I’m stuck on the problem of removing manual configuration for the classes that call super.loadConfiguration(). I will try to explain.

There is module ApplyConstructAbstractModule

class ApplyConstructAbstractModule extends AnnotatedAbstractModule 

Some modules extend this class to have configured fields, such as constructQueries, isReplace, and parseText.

for example this module:

class ApplyConstructWithChunkedValuesModule extends ApplyConstructAbstractModule

As it was before, the ApplyConstructWithChunkedValuesModule called super.loadConfiguration(), which was overridden in ApplyConstructAbstractModule to manually configure the fields.

However, when I remove this overriding method from the parent, I’m unsure how to configure it from the child class using annotations. If we call super.loadConfiguration() from the child, it will see that the parent did not implement it and will then call loadConfiguration from AnnotatedAbstractModule, which only configures the child class.

Therefore, I’m not sure how to resolve the problem without manually configuring all the fields in ApplyConstructAbstractModule within the loadManualConfiguration() method and then calling it from the child modules.

[blcham]

I’m not sure why, but when I try to test locally with mvn clean install, I get the same error that we solved in a previous ticket.

Maybe you have local changes there, try this to be sure:

git clone -b 274-load-configuration-parallel 'https://github.com/kbss-cvut/s-pipes'
cd s-pipes
mvn clean install

---

Therefore, I’m not sure how to resolve the problem without manually configuring all the fields in ApplyConstructAbstractModule within the loadManualConfiguration() method and then calling it from the child modules.

Yes, that is also option.

But how about reimplementing loadConfiguration() to process also parent classes? Would it solve the issue?
I mean to process @parameter annotations of all classes from something like this.getClass().getSuperclass().

[palagdan]

But how about reimplementing loadConfiguration() to process also parent classes? Would it solve the issue?
I mean to process @parameter annotations of all classes from something like this.getClass().getSuperclass().

@blcham

Thank you for the idea! It should be better than doing it manually.

[blcham]

Almost done, see my comments

[blcham]

LGTM, you can merge it without change, just see my comment about recursion