cache fixture results per sequence, clearing the cache at the end

[acoover]

No description provided.

[acoover]

fixes #13

[acoover]

This change removes the ability to override dependency injection for fixtures by providing explicit values for parameters. @domanchi can you provide some clarity on the motivation behind that functionality?

[domanchi]

@acoover: when I envisioned this tool, I imagined fixtures being flexible enough to exist with other helper functions. This means that it supports manual fixture invocations (e.g. if you wanted to specify a precise value, or use it just as another factory to create data).

I don't think our internal use of this tool requires this manual invocation though.

[domanchi]

This looks functionality sane to me; I'm just wondering more of the design aspects of these decisions.

[domanchi]

Hmm.

I wonder whether this should be a sequence level cache, rather than a request level cache. My thinking around this is that fixtures exist to complement the "traditional" CRUD application. What is now created in a fixture would have been created with a separate request, if we were testing a full CRUD application. This means that technically, it would have gone in the sequence level cache that was enabled here: #10

Granted, since these fixtures are executed on "request setup" time, rather than caching the responses, this may be practically difficult to do.

[acoover]

One bit which is off about placing the call to clear the cache here is that the cache is cleared before we make the request to the server to check for vulnerabilities, which means we don't use the same resources for the victim and attacker accounts. This shouldn't matter since the execution path to setup the fixtures is the same, but it seems like we should clear the cache after we analyze_requests

[acoover]

Looking at #10, I don't think we can leverage a per-sequence cache in the same way as used in that PR. My reasoning is that in addition to a FuzzingRequest reading cached values when generating parameters for the request in fuzzer.py, we also need to access the cache within the fixtures themselves. If we didn't allow for dependencies between fixtures (fixture A takes as input the values from fixture B) we could generate all fixtures upfront before running a request sequence then toss the values in a local dict used to generate params as in #10 - the benefit here would be to rely on garbage collection to invalidate the cache as opposed to the gross explicit clear_cache in this PR.

[domanchi]

Yeah, it's tricky, and I don't know a better way forward right now.

Let's ship this, and punt on figuring out how to better structure it. After all, we don't need to support manual invocations anymore.

EDIT: Creating #19

[acoover]

@acoover: when I envisioned this tool, I imagined fixtures being flexible enough to exist with other helper functions. This means that it supports manual fixture invocations (e.g. if you wanted to specify a precise value, or use it just as another factory to create data).

I don't think our internal use of this tool requires this manual invocation though.

The fixture dependency-injection is magical enough that the additionally complexity required to support manual invocation doesn't seem worth the effort. My feeling is that most fixtures will alias other functions which perform the bulk of the work to setup the fixture. That is to say, most fixtures look like this

@fuzz_lightyear.register_factory('foo')
def foo():
   return foo_factory(bar=1)

Allowing foo to be invoked directly is of little value over just invoking the foo_factory method.

As a datapoint, pytest explicitly disallows fixtures to be invoked directly.