Add support for overriding values

joachimvh commented 2 years ago

Closes https://github.com/LinkedSoftwareDependencies/Components.js/issues/66 Does exactly what I want to do in the 2nd part of https://github.com/CommunitySolidServer/CommunitySolidServer/issues/1348.

This is my suggestion on how to allow values to be replaced in Components.js. Every time the value for a parameter gets requested, now it first checks if there is a potential override value. This does partially break the RDF interpretation of these objects, so suggestions there are welcome if it somehow can be improved, but changing the identifiers would make this feature useless.

I wanted to make this a ParameterPropertyHandler, but that does not allow the recursive call to the same function again so had to put it in there.

Added the override predicate to the oo ontology just so I had a predicate, but this can be moved/renamed to wherever it fits best.

If accepted, documentation about this should probably also be added somewhere but not sure where.

As an aside, I noticed all the integration test modules still use a 4.0.0 context.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 2782318068

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

101 of 101 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 2356169745:	0.0%
Covered Lines:	1361
Relevant Lines:	1361

💛 - Coveralls

rubensworks commented 2 years ago

I definitely see the value of this, but I'm not sure if this approach is flexible enough.

The problem (as you documented in the code), is that you can only do a single override with this approach. While I can imagine that overriding may be useful when you may want to do a chain of overrides.

Do you think something in the direction of my earlier suggestion would be a good alternative?

joachimvh commented 2 years ago

The problem (as you documented in the code), is that you can only do a single override with this approach. While I can imagine that overriding may be useful when you may want to do a chain of overrides.

Well you can do multiple but then you depend on the order Component.js loads the triples. Which is deterministic atm but perhaps not a good idea to rely on.

I also thought about perhaps adding weights, so you could always use a larger weight than the previous ones if needed. Or we could give the override object an identifier so you can override the override (although that might get messy).

In practice I'm not sure how often it would be necessary to have multiple overrides on the same parameter of a class, as usually there is one larger config that you want to make some changes to.

Do you think something in the direction of my earlier suggestion would be a good alternative?

The problem with that solution is that it does not solve the problem I want since it introduces a new identifier with different values, instead of changing an existing identifier. As mentioned in that issue, a common use case is people wanting to change the timout of the locker, which after this change they would be able to do with just 3 extra lines in their custom config. If instead they got a resource with a different identifier they still would have the original problem of having to also update all configs that reference the original identifier.

rubensworks commented 2 years ago

Or we could give the override object an identifier so you can override the override (although that might get messy).

This may actually be a good solution.

We could for example structure overrides via something like this:

{
  "@id": "ex:myObjectOverride",
  "@type": "Override",
  "overrideInstance": "ex:myObject",
  "overrideParameters": {
    "hello:hello": "BETTER WORLD"
  }
}

This would cause the ex:myObject to be overridden, but ex:myObjectOverride to also be a virtual representation of this instance that can be overridden again.

Since overrides now have identifiers, they could be chained, by adding them as value to overrideInstance.

What do you think of this approach?

joachimvh commented 2 years ago

What do you think of this approach?

Yes that would also be fine for me since it accomplishes the same goal. Would have to look into how much more complex this would be to implement though. The advantage of the current solution is that it is very easy to find the override value since it's linked to the object being changed. Doing it like this would require going over the entire config to find all the overrides and then find the one that might be applicable. 95% of this PR was me finding out how things worked internally, the code itself was nothing after that 😅.

There also is no path anymore from the entry point identifier to this override object, which might also break some things in components.js.

rubensworks commented 2 years ago

Indeed, implementation will be a bit more complex.

I suspect we'll need a preprocessing step that collects all overrides (accumulated in a single one if chained), and then they can be applied later on in the ParameterHandler (similar to what you already have).

joachimvh commented 2 years ago

Another disadvantage that I can think of is that this solution also makes it more complex to override an override. E.g., to override your example, you would need:

{
  "@id": "ex:myObjectOverride2",
  "@type": "Override",
  "overrideInstance": "ex:myObjectOverride",
  "overrideParameters": {
    "overrideParameters": {
      "hello:hello": "BETTER WORLD"
    }
  }
}

Right? And another nested stack for override 3, etc. Unless we make Components.js smart enough to know that if it overrides an override it should apply the parameters to its target directly.

rubensworks commented 2 years ago

Another disadvantage that I can think of is that this solution also makes it more complex to override an override.

I guess that could work like this:

{
  "@id": "ex:myObjectOverride1",
  "@type": "Override",
  "overrideInstance": "ex:myObject",
  "overrideParameters": {
    "hello:hello": "BETTER WORLD"
  }
}

{
  "@id": "ex:myObjectOverride2",
  "@type": "Override",
  "overrideInstance": "ex:myObjectOverride1",
  "overrideParameters": {
    "hello:hello": "EVEN BETTER WORLD"
  }
}

No nesting would be needed then.

joachimvh commented 2 years ago

Having a look at where and how to implement this. The fact that there is no link from the resource that is being modified to its override makes this a bit annoying (if you want to be efficient and avoid doing the same thing multiple times).

At some point we need to find a path of Override objects, chaining them all together from the latest one to the eventual target resource. This only needs to happen once, but can only happen after all configs have been registered. The first time we are sure this is the case is when calling ComponentsManager.instantiate. Doing it earlier would mean the configs have not been registered yet, but doing it later would mean it happens somewhere that is called multiple times.

So I'm thinking of adding a function there that generates the necessary metadata and then passes it as an additional parameter to ConfigConstructorPool.instantiate. Adding it as a new entry to IConstructionSettings might be interesting since this would also allow overriding when calling the instantiate function without needing to define the override in a config.

This would generate metadata for all the overrides though, not just for those that might be needed for instantiating the requested identifier. This means that an error can be thrown for an override that might not even be relevant for the requested instantiation. E.g., if it is targeting an unknown parameter, has multiple targets instead of one, or there is a cycle of overrides.

rubensworks commented 2 years ago

Adding it as a new entry to IConstructionSettings might be interesting since this would also allow overriding when calling the instantiate function without needing to define the override in a config.

That might be a bit too late in the pipeline. I would suggest adding it as a preprocessing step (see preprocess package). So it would end up being called here: https://github.com/LinkedSoftwareDependencies/Components.js/blob/db401883fb51f08230887cc54ae051b5ab917824/lib/construction/ConfigConstructorPool.ts#L88-L100

However, the config processors are currently designed in such a way that only a single one can be invoked. So we may have to modify this slightly so that a config processor can indicate whether or not they want preprocessing to continue (which is something the override preprocessor will want to do).

joachimvh commented 2 years ago

That might be a bit too late in the pipeline.

From what I can tell the place I'm suggesting is earlier in the pipeline since the call order is ComponentsManager.instantiate -> ConfigConstructorPool.instantiate -> ConfigConstructorPool.getRawConfig. So to clarify, do it before this call here: https://github.com/LinkedSoftwareDependencies/Components.js/blob/43c631ba1e832e770449591803ce03bdb2f74747/lib/ComponentsManager.ts#L59 And then pass this generated map which links identifiers to their override values as an extra parameter there (through the settings parameter)

The reason I didn't want to add it in the ConfigConstructorPool is because that one gets called recursively for every object that needs to be instantiated, while the list of overrides only needs to be generated once by going over all resources in the objectLoader and finding those with the type Override.

joachimvh commented 2 years ago

In the interest of the discussion I have pushed a new solution. It still needs cleaning up and refactoring so don't mind that. The solution works though (for the override test, breaks many other tests 😄).

It now generates an overrides object that contains for every relevant identifier, a mapping between a parameter identifier and the new Resource to use there. This happens in the ComponentManager when calling instantiate.

As per your comment above a new ConfigPreprocessorOverride was added that returns a new object with the parameters updated.

It's mostly this last part I'm not sure about what would be best. Currently I'm creating a new object so I don't modify the original resource in the object loader. Problem is that this new object will then have a different ID which would make generated errors less clear to read, and I'm guessing would cause issues if there are circular dependencies somewhere.

I did notice that the ConfigPreprocessorComponent modifies some of the Resources in place, which makes me wonder if it would be OK to also do this for the overrides. If yes, this could also happen in the same place the overrides are currently calculated, or it could still happen in a preprocessor so only the components relevant to the instantiation are changed.

joachimvh commented 2 years ago

Another day, another location to put the overrides. This version moves everything to the config preprocessor and caches the found overrides there.

The one thing that is missing is a way to indicate that the override cache should be recalculated. Through what component would you want to expose that? Will probably require some fiddling since that call will need to reach the new config preprocessor.

joachimvh commented 2 years ago

I guess we can expose this within the ComponentsManager. Perhaps a method like resetOverrides? (I assume resetting is sufficient, as it will be lazily recalculated?)

Just resetting the internal variable to undefined should make it all work again yes.

The issue I had with exposing is that there is no direct link anywhere from the ComponentsManager to the override preprocessor. Should this preprocessor be an extra constructor parameter for the ComponentsManager then so it can call that preprocessor directly? Could also add a new reset function to the preprocessor interface, and to the config constructor pool (where the latter calls the first on all its preprocessors).

rubensworks commented 2 years ago

The issue I had with exposing is that there is no direct link anywhere from the ComponentsManager to the override preprocessor.

Right, we should avoid too much hard-coupling.

Could also add a new reset function to the preprocessor interface, and to the config constructor pool (where the latter calls the first on all its preprocessors).

That sounds good to me!

joachimvh commented 2 years ago

Cleaned this up to make it an actual complete PR.

Only thing that is missing is documentation, wasn't sure if this needs to be added here or the documentation repo.

joachimvh commented 2 years ago

Did the changes, will look into doing a PR in the doc repo

rubensworks commented 2 years ago

Released as 5.3.0

LinkedSoftwareDependencies / Components.js