Update and standardize processor parameters to support adding validate_query to all processors#619
Update and standardize processor parameters to support adding validate_query to all processors#619dale-wahl wants to merge 23 commits into
parameters to support adding validate_query to all processors#619Conversation
…rameters in database
…cks (protect against extensions)
…) from validate query.
… being passed to processors
|
@stijn-uva We discussed this one in person. As all things I touch, they grow. This was actually pretty illuminating and fixes a fair number of bugs. Mostly with sensitive parameters and some drift in options presets were passing to processors. I added warnings and a dev only mode that we may want to run on our server which should warn us of that sort of drift. I think it is solid. I am going to build on it with a |
This PR standardizes the
validate_querymethod so it can be used across all processors instead of just datasources. It is still required for datasources — now enforced at the queue endpoint (a datasource that only inherits the default pass-through is refused with a clear error, instead of the old rawNotImplementedError) — but NOT required for processors.BasicProcessornow provides a pass-throughvalidate_queryby default, so a processor opts in only when it has something to check or confirm. (We could require it on processors too, but all extensions would need updating, so in the off chance others have created their own, we'd need to deprecate and properly inform them somehow.)In order to really do this properly, I reviewed
self.parameters, thequeryargument toget_items(which only exists forSearchclasses aka datasources), andget_parameterswhich did not always return the same exact thing. #603Core
BasicProcessor: added a defaultvalidate_queryand aget_validated_querywrapper that both queue endpoints now call instead ofvalidate_querydirectly. This makes it the single place any check/rule applies no matter where a dataset is created from. Right now it (a) strips framework-injected transient keys (TRANSIENT_QUERY_KEYS, i.e.frontend-confirm) from the stored result so they don't clutter the record, and (b) warns when avalidate_querystoresNonefor a key that wasn't in the submission (a common mistake invalidate_querymethods via"never_submitted": query.get("never_submitted")when rebuilding a query).BasicProcessor: addedoption_givento check that a user actually submitted a specific option vs it being filled with the default viaget_options. Backed by a snapshot of the stored parameters taken at run start.UserInput.parse_all: now respectsrequirements_met(an abstraction of the existing_requirement_met) so that only user seen/chosen options are stored in the database. This makes the UI more truthful (options never seen by a user showed up as parameters in the database previously) and enables theoption_givenmethod above. It also closed a hole where an unmet-requiresoption whose hidden field wasn't submitted got its default stored anyway.Sensitive parameters
I found some gaps in how these were handled and actually some are still in our database (need to take care of that in migrate script on next release).
DataSet: addedremove_sensitive_parametersto standardize and allow use of the method in different places (checks both the producing processor's and the current-type processor's options, in case a filter adopted a new type).DataSet: addedremove_sensitive_parameters_from_next, which recursively strips sensitive values from queuednext/preset chains. This was the real leak — a datasource/preset that embeds a follow-up'sapi_key(e.g.audio_to_text,annotate-images) left it in the record forever. Called after the follow-ups are created, so they get their own copy first.ThingExpirer: addedexpire_sensitive_parameters, which deletes sensitive parameters (top-level and nestednext) for unfinished datasets over 7 days old. They should already be deleted, so this is just a fallback safety measure. (Basically would catch a DataSet with sensitive info that cannot run/crashes. Would catch a disabled datasets in limbo.)api_keysensitiveongoogle-vision-apiandannotate-imagesand dropped their manualdelete_parametercalls (fragile — a crash before that line leaks the key).Dev-only parameter lint
Noticed some old params in presets and used this to find them. Enabled via control panel (possible point for extension or other dev gating).
BasicProcessor.warn_unexpected_parameters: warns (never raises) when a dataset is queued programmatically (preset pipelines,nextchains) with a parameter its target processor doesn't accept — the thing that silently drops a preset's mistaken or misdirected key. Called at the two programmatic doorways (after_process'snextloop andProcessorPreset.process). Exempts framework keys (FRAMEWORK_PARAMETER_KEYS),_-prefixed internal plumbing, and a per-processoraccepted_parameterstuple ("keys I read but don't always offer as an option" — e.g. a config-gated option a preset deliberately passes).dev.modeconfig setting (a live-toggleable "Developer mode" toggle, off by default.Tests
test_option_declarations— fixed 12 processors whose defaults were the wrong type or not a valid choice), and any worker definingvalidate_querymust make it a@staticmethod(test_validate_query_declarations). Plus coverage for gated-option storage, the wrapper, the nested sensitive scrub, and the parameter lint.Bug fixes surfaced along the way
The dev lint (and the option-declaration checks) turned up a set of real bugs, now fixed:
annotate-imagesqueuedconvert-vision-to-csv, a processor type that does not exist (it'sconvert-google-vision-to-csv), so its final CSV-conversion step silently never ran.similar-wordspassedtimeframetotokenise-posts, which readsdocs_per— so the timeframe selection was silently dropped.neologisms,annotate-images) and the redundant reversed-date-range checks in several datasources (parse_allalready raises on those).audio_to_textcopied its whole merged option set into the follow-up viaquery.copy(); now filters to the audio processor's own options.Discussion:
remove_sensitive_parameters+ThingExpirer) that could remove sensitive parameters after a dataset was run and properly finished. We could therefore delete sensitive parameters then and allow interrupted and/or crashed processors to retry w/ them. We could tighten theThingExpirerin that case to cleanup quicker. (This PR keeps the original behavior — deletion at run start, i.e. immediate removal from the DB — as the default.)queryinvalidate_query(query)!= thequeryinget_items(query). Always the case, but before thisquerywas NOT re-hydrated viaget_optionsand thus differed fromself.parameters. Sincequerydoes not exist inBasicProcessor.process()and is only available inSearch.get_items()this difference was meaningful, potentially confusing, and could result in unusual behavior. This PR fixes that by aligningqueryandself.parameters(andget_parameters). The newoption_givenmethod can now be used to check if a option key actually was provided by the user. FixesSearch.get_items():queryvsself.parametersdivergence #603.self.parametersto provide this information (use requirements met to ensure the user actually made a decision/choose a default), but it requires a lot of diligence on developers to understand thatself.parameters.get('some key', "meaningful value with key absent"). There are many places in our code where that default is not the same as the declared default inget_optionsand there are a few places where we useself.parameters['some key that may no longer be there']which would crash (that's actually a benefit, but would require behavior change on our part as keys have been always present inself.parameters). Soself.parametersstill fills every declared option, andoption_givenis the explicit way to ask "did the user provide this?".Future fun:
self.parameters.get("key", "meaningless default often conflicts declared default anyway"). If it is inget_options, the key has always been inself.parameters(always defined as "many years at least"). Could even be replaced withself.parameter["key"]to actually throw if we mistype or change an option key.result-metadata.htmlhandling of parameters. The parameters going forward will only be those accepted by the user. It thus becomes useful to include non-truthy parameters as they were a decision. (And the long hide certain keys possibly should be defined somewhere else and could respectsensitiveandcacheattributes.)please ignore misfortune branch name typo