[Pulp-dev] 3.0 Validation update

[Sean Myers]

On 04/11/2017 03:49 PM, Austin Macdonald wrote:
> Where should business logic live? As an example, I want to consider the
> sync task [0] and the need to ensure that an importer is syncable. For now,
> let's say that an importer is syncable if it has a feed_url.
> 
> Since the call to sync an importer comes from the API, the earliest we can
> check that the configuration is syncable is in a not-yet-written SyncView.
> Checking syncability here is ideal because it allows us to immediately
> return a meaningful http response instead of happily returning information
> about a Task that we already know is doomed to fail.

This is the crux of the issue, for me. We *don't* know that the task is already
doomed to fail. Because concurrency, it's both possible for an invalid importer
to become valid before the sync task is run, and it's possible for a valid
importer to become invalid before the sync task is run. I appreciate the desire
to be "nice" to users, but argue that a consistent API is nicer than an
inconsistent API. To be consistent here, I think it's reasonable to always
always always return a task pointer, and let the state of the importer be found
to be valid or invalid *at the moment the sync is attempted*. Then any integration
is always: call the sync, track the task id you get back, which reports error or
success. Doing validation outside of the task makes the workflow: call the sync,
check for error?, then (again) track the task id you get back which reports error
or success. We're trading the appearance of usability for increased complexity
in our code (and therefore risk) and difficulty of integration in the user code.

> Performing the check in the API layer is not enough. We have discussed edge
> cases that lead to an importer's feed_url being removed while the sync task
> is in the WAITING state. To make an assertion to the plugin that the
> feed_url exists, we have to check syncability again when the Task moves
> into an active state.

...or we could only check when the Task is run, and make sure that when tasks
fail they always provide *great* error messages. :)

> My thinking is that we should put this business logic on the model.
>
> Admittedly, it is not a clean fit with the separation of concerns
> philosophy but we have already violated the concept by putting the sync
> method on the model. If sync is on the model, it seems like ensure_syncable
> should be too.

A few points here. First, "we've already broken the rules" is *never* a
good reason to declare carte blanche and throw out the rulebook. Yes,
there may be exceptions, but we should endeavor to ensure those cases
remain exceptional and don't become the norm.

The "sync" method is not declared on the Importer model in platform. It's
exposed as the entry point into sync on the Importer model exposed as part
of the plugin API. Six of one, half-doze of the other? Maybe! But I posit
that we're maintaining a separation of concerns by explicitly not handing
the platform Importer model directly to the plugin API, and by not
implementing sync and publish directly on the platform app models. We created
the plugin API Importer and Publisher proxies specifically to create this
separation, placing trust in plugin developers to stay within the plugin
API while still allowing platform the freedom to make backward-compatible
changes to the plugin API as it sees fit.

It's worth noting here that we have not yet defined exactly what the semantic
versioning contract is in the plugin API, and how it applies to these exposed
models. The only thing that can be said definitively is that we separated them
out with the specific intent of separating this business logic from the
platform models without creating a burdensome interface in the process. The
concerns are separated.

> If we write the platform API layer to use this kind of business logic, then
> the plugins can add double checking business logic without modifying the
> API and Task layers.

On the other hand, if we ignore this handling of business logic in the platform
code, then plugins become entirely responsible for their own validation inside
the sync method of their importers, receiving the benefits of a simple interface
without the confusion of platform shenangangs doing who knows what before and
after the sync call that may well be detrimental to a given plugin's workflows.

With complexity comes bugs. I say keep it simple, *especially* for the MVP. If
we hate it, we can absolutely add "task pre-validation" as a feature in 3.y.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 866 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20170411/9989d60a/attachment.sig>