dissect.target icon indicating copy to clipboard operation
dissect.target copied to clipboard

Explore alternatives to namespaceplugin class

Open twiggler opened this issue 5 months ago • 4 comments

Introduction

The current implementation of NamespacePlugin has a couple of problems.

Firstly, its name is confusing; how does it compare and contrast with the class attribute __namespace__?

Secondly, it arguably misuses the class hierarchy to aggregate child plugin functions by means of monkey patching.

While the old system for grouping / aggregating functions required some boilerplate, at least it was very explicit.

Thirdly, I have a gut feeling we had quite a number of bugs involving this code, though I don't have any data to back this up.

Brainstorm: ideas

We could add an annotation @aggregate for every plugin function that needs to be aggregated over a group of functions, for example:

class ChromiumPlugin:
    @export(record=BrowserHistoryRecord)
    @aggregate(group="histories")
    def history(self) -> Iterator[BrowserHistoryRecord]:
       pass

class EdgePlugin:
    @export(record=BrowserHistoryRecord)
    @aggregate(group="histories")
    def history(self) -> Iterator[BrowserHistoryRecord]:
       pass

This would make available a function called browser.histories, which does not reside in a plugin. Because our function descriptors FunctionDescriptor are currently always tied to a plugin through the module attribute, it is probably best to create a AggregateDescriptor like:

@dataclass(frozen=True, eq=True)
class AggregateDescriptor: 
    group: string
    items: FunctionDescriptor[]   # Functions to aggregate
   ...  

Then, the function resolver(s) such as find_functions should be modified to also search AggregateDescriptors.

The aggregator function itself can be a generated by a higher order function by invokes all the functions (items), and aggregating the results.

Limitations / open questions

  • How do we add documentation to the aggregations? We could pass a docstring to the decorator.
  • How can the user specify how to aggregate? By default, we concatenate all records. But actually I don't see any other ways than concatenation.
  • How would this work with cold starts of the plugin system?

There might be other solutions, will continue to think about it.

If none are acceptable, I would rather go back to the "old" system, after consulting with our users.

twiggler avatar Jul 15 '25 07:07 twiggler

The NamespacePlugin class, I presume?

Schamper avatar Jul 15 '25 07:07 Schamper

Thirdly, I have a gut feeling we had quite a number of bugs involving this code, though I don't have any data to back this up.

I am not familiar with the inner workings of the namespace implementation, but I believe several of the issues listed below may be related to namespace plugins:

  • https://github.com/fox-it/dissect.target/issues/822
  • https://github.com/fox-it/dissect.target/pull/194
  • https://github.com/fox-it/dissect.target/pull/711#issuecomment-2312296522
  • https://github.com/fox-it/dissect.target/pull/763

Zawadidone avatar Jul 16 '25 11:07 Zawadidone

The biggest problem with the current implementation is this: https://github.com/fox-it/dissect.target/issues/1015

Schamper avatar Jul 16 '25 12:07 Schamper

The benefit of having some sort of flexibility to "implement" stuff in the "aggregator" namespace is that you can provide some additional helpers or convenience functions, example:

Also with an @aggregate on each export it's becoming super verbose and error prone to add that to every export. Maybe just a __aggregate__ attribute on the class?

class EdgePlugin:
    __namespace__ = "edge"
    __aggregate__ = "browser"

    @export
    def history(self): ...

You could still have a separate browser plugin, and the plugin system could sew the exports together:

class BrowserPlugin:
    __namespace__ = "browser"

    @internal
    def keychain(self): ...

# t.browser.keychain() works
# t.browser.history() works and is "injected"

Schamper avatar Jul 16 '25 12:07 Schamper