Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Mandatory ManagedProviders #3017

Open
J-N-K opened this issue Jun 26, 2022 · 10 comments · May be fixed by #3640
Open

[RFC] Mandatory ManagedProviders #3017

J-N-K opened this issue Jun 26, 2022 · 10 comments · May be fixed by #3640

Comments

@J-N-K
Copy link
Member

J-N-K commented Jun 26, 2022

@openhab/core-maintainers The AbstractRegistry defines the presence of a managed provider as optional. IMO we should change that and require a managed provider. This simplifies code (because we don't have to check if the provider is present when adding/removing elements) and we have implemented managed providers for ever registry anyway. WDYT?

I would be willing to implement that change.

@kaikreuzer
Copy link
Member

I do not really see any reason either why we couldn't make it mandatory by now.
@cweitkamp and @wborn What about you?

@wborn
Copy link
Member

wborn commented Jul 10, 2022

It makes sense to me to require at least one managed provider for a registry nowadays.

@splatch
Copy link
Contributor

splatch commented Jul 11, 2022

I don't think that introducing requirement for a managed provider on all elements is fully justified. You can have a read only (managed) installation which does work with providers and registry elements which are supplied from elsewhere - through files or central configuration server and no expectation to supply new elements at all.
While this is not how openhab distro works it doesn't mean that such situation doesn't happen. Given that all registers rely on same base class change in this area will pretty much default everything to wait for storage interface to come up.

I believe main problem is somewhere else - a REST layer and UI can not reliably determine if a Thing is read only or not (REST is doing it through registry lookup), same for links, rules and so on. Why not making an accessor or interface (Modifiable#isModifiable?)) for core representations to streamline that part? This could be set by Provider itself without a hard distinguish between ManagedProvier or plain one.

@J-N-K
Copy link
Member Author

J-N-K commented Jul 12, 2022

I thought about that, too. But what happens if we have two modifiable providers and add a new entity? Adding it to both is obviously wrong, but what is the criteria to choose one over the other? Since there is no deterministic order in which providers are added, it‘s not easy to decide.

@splatch
Copy link
Contributor

splatch commented Jul 12, 2022

I thought about that, too. But what happens if we have to modifiable providers and add a new entity?

Each provider produce somehow a thing, a link and so on, so update can be actually an callback which is injected by a provider. In theory, if we would be following "domain driven design", it would be considered a recommended pattern. With current codebase and how it is shaped it is a bit awkward.
Anyhow - getting that way will drastically simplify registers but will shift more logic to the providers as they will have to coordinate update logic. An interesting aspect of such approach is that you then you could be switching a typical json storage based provider into managed or not by single configuration parameter.

@J-N-K
Copy link
Member Author

J-N-K commented Jul 12, 2022

I‘m not sure I understand what you say. User adds a new thing (item/whatever) via UI. UI makes a REST call and the thing now needs to be added. How does the system chose which provider shall be used if there is more than one modifiable provider?

Currently the REST resource adds it to the registry and the registry forwards this addition to the managed provider (if available).

@splatch
Copy link
Contributor

splatch commented Jul 12, 2022

Lets speculate a little bit. Below code is a pure example of how update logic could be shifted to a lower layer which actually does the work with updating the definition, while retaining a feedback loop for interested parties.

Thing and ThingBuilder:

class Thing {
  Function<Thing, Thing> onUpdate;
  void onUpdate(Function<Thing, Thing> updateHandler) {
    if (onUpdate != null) {
      // first onUpdate handler is guaranted to be one handled by provider so we must call it first,
      // later handlers will always receive definition merged by thing source
      this.onUpdate = this.onUpdate.andThen(updateHandler);
    }
    // update will no happen, ignore handler
  }
  boolean isModifiable() {
    return onUpdate != null;
  }
}

class ThingBuilder {
    ThingBuilder onUpdate(Function<Thing, Thing> updateHandler) {
       // function input is new thing definition, returned value is updated thing definition
       this.updateHandler = updateHandler;
    }
    ThingBuilder onDeleteConsumer<Thing> deleteHandler) {
       // a callback for deletion of things
       this.deleteHandler = deleteHandler;
    }
    Thing build() { ... } // make sure handlers are passed to a thing instance
}

Then rest layer could be as simple as this:

Thing thing = thingRegistry.get(thingUID);
if (thing.isModifiable()) {
  thing.update(newDefinition);
} else {
  throw new WebApplicationException();
}

Assuming some provider:

class JsonThingProvider implements Provider<Thing>, ThingProvider, Function<Thing, Thing> {
  Collection<Thing> getAll() {
     return getJsonStream()
        .map(json -> thingBuilder.newBuilder(json))
        .map(builder -> if (!readonly) builder.onUpdate(this).onDelete(this))
        .toList();
  }
  Thing apply(Thing update) {
    // merge definitions
    return updated;
  }
  void accept(Thing deleted) {
     things.remove(deleted.getUID);
  }
}

And a ThingRegistry or actually an AbstractRegistry logic related to that:

class ThingRegistry extends AbstractRegistry<ThingUID, ThingProvider> {
  Collection<Thing> getAll() {
     return getProviders()
        .flatMap(Provider::getAll)
        // appends an additional callback to provider one, if specified
        .map(thing -> thing.isModifiable ? thing.onUpdate(this))
        .map(thing -> thing.isModifiable ? thing.onDelete(this))
  }
  Thing apply(Thing merged) {
    this.things.put(merged.getUID(), merged);
    return merged;
  }
  // for deleted just untrack removed element
}

@kaikreuzer
Copy link
Member

Coming back to the original question: I don't really see a good argument for keeping or even extending the complexity that we have - our managed providers always come for each registry with the same bundle and it imho makes much sense to couple them now. It removes quite some code and simplifies the system's lifecycle.

@ccutrer
Copy link
Contributor

ccutrer commented Nov 21, 2022

And backing up to the tangential discussion on having multiple managed providers, and allowing the UI to choose which provider to use of multiple managed providers, as well as knowing if the object is read-only...

This is pretty much exactly what I've been working on in the Ruby scripting helper library lately. If you'll look closely at ManagedProvider, you'll notice that there's nothing that says it must be backed by a Storage. I'm taking advantage of this fact so that I can create items (and things and metadata) in Ruby under a new provider that implements ManagedProvider. The elements in these providers are transient - the entire provider gets removed when the script unloads (similar to how ScriptedRuleProvider/RuleSupportRuleRegistryDelegate currently keep track of rules from scripts, and remove them when the script unloads). I still allow the user to choose the default ManagedProvider (Registry.getManagedProvider) if they would like (metadata especially) the objects to persist after the script unloads. I currently have my own mechanism for specifying the default provider (a thread local variable, but that wouldn't make sense for the REST API/UI), but because my own provider implements ManagedProvider, and the default ManagedProvider implement it, the rest of the code doesn't care which provider it's using. It just grabs the current default provider, and calls add on it.

As for checking read-only-ness... my remove method (or updates), instead of just blindly trying to remove from the default managed provider for a registry, I've added a providerFor method that takes a key, and looks up which provider provided it in Registry.elementToProvider, and then removes it from that provider. It does a safety check that the provider associated with the element is a ManagedProvider (and thus implements remove). It would be easy enough to add a isManaged predicate method to a registry as well, for the UI to more directly know if an element is read only. But... funny story on this... the UI does not know if metadata is read only. If you have an item defined in an .items file that has metadata (thus provided by GenericMetadataProvider), the UI will still show that you can edit it. It will even show that it worked, except that it didn't, but in the log you can see a warning that AbstractManagedProvider rejected the update because it doesn't exist from that provider.

Anyhow, #3170 is semi-related, just cleaning stuff up for ScriptedRuleProvider to more fully match that paradigm.

And, finally, back on topic - +1 making getManagedProvider() not return an Optional. I've forgotten the get() call on that a lot in the past few months.

@splatch
Copy link
Contributor

splatch commented May 29, 2023

I been recently experimenting with NetworkInterfaceProvider and NetworkInterfaceRegistry; there is fair chance that this registry will not have a ManagedProvider behind, because it requires further privileges for software. Even if its granted to openhab user it is still a conditional use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants