feat: add dynamic url info#998
Conversation
| super().setup() | ||
| for rule in self.rules: | ||
| rule = typing.cast(GenericAdderRule, rule) | ||
| rule.init_generic_adder(self._job_tag_for_cleanup) |
There was a problem hiding this comment.
I wonder if we really need this external init and would like to avoid it. The list_comparison has it, as the list_search_base_path comes from the processor and is relevant for the function of the rule. Here rule could generate it's own job tag, couldn't it?
There was a problem hiding this comment.
I wonder if we really need this external init and would like to avoid it. The
list_comparisonhas it, as thelist_search_base_pathcomes from the processor and is relevant for the function of the rule. Here rule could generate it's own job tag, couldn't it?
Yes we do, I forgot to add the _shut_down function here, this makes it clearer why we need to init here
There was a problem hiding this comment.
Mh, I see where you are coming from. Alright, we can keep this for consistency with other processors (namely list_comparison).
But I still think that the job_tag and managing callbacks is rather an implementation detail of the rule, which it should handle itself. I think it would be consequential, that processor.setup() calls rule.setup() and processor.shut_down() calls rule.shut_down(), whereas managing callbacks is completely handled by the rule.
Currently we are calling an init (which has setup-like semantics) on the rule but basically handle the shutdown on the processor-level, which is inconsistent.
| from logprep.util.defaults import ENV_NAME_LOGPREP_GETTER_CONFIG | ||
| from logprep.util.getter import HttpGetter | ||
|
|
||
|
|
There was a problem hiding this comment.
test_cases and failure_test_cases would be nice here as well. I figure that this is a scope creep; so probably we drop it for this PR
There was a problem hiding this comment.
test_casesandfailure_test_caseswould be nice here as well. I figure that this is a scope creep; so probably we drop it for this PR
Yes thats right, but I would rather have this be in another PR / task of itself
e0b8071 to
1ab8eeb
Compare
| def __attrs_post_init__(self) -> None: | ||
| if not self.target_field and not self.target_field_mapping: | ||
| raise ValueError("adding values from url requires target_field or target_field_mapping") |
There was a problem hiding this comment.
This is does not have a super class, but we need to ensure all derived classes actually call super attrs_post_init. I wonder if we can just always have it (even for non-derived classes), but I have not tried it and out this might throw an error. Try it out if you feel like it, otherwise resolve as a no-op.
| def __attrs_post_init__(self) -> None: | |
| if not self.target_field and not self.target_field_mapping: | |
| raise ValueError("adding values from url requires target_field or target_field_mapping") | |
| def __attrs_post_init__(self) -> None: | |
| super().__attrs_post_init__() | |
| if not self.target_field and not self.target_field_mapping: | |
| raise ValueError("adding values from url requires target_field or target_field_mapping") |
| @@ -160,20 +213,47 @@ def _refresh_add(self): | |||
| def __attrs_post_init__(self): | |||
There was a problem hiding this comment.
super needs to be called here
| # TODO: This never gets cleaned up, Memory leak on a lot of new generic adders / generic resolvers | ||
| getter.add_callback(f"generic_adder:{self.id}:{add_file}", self._refresh_add) |
There was a problem hiding this comment.
We have the job_tag now and can resolve this properly
There was a problem hiding this comment.
Not here we dont, because that is the rule config and gets loaded on config time, we dont pass in the job tag, but I guess I could just change that with a private _callback_tag on the config, pass that into the rule from the processor and then also remove the init because now the rule knows the callback tag?
| mapping_source_field, | ||
| mapping_target_field, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
If we stick to this, this might be a case for a processor warning
2 participants
Description
Assignee
Documentation
Code Quality
How did you verify that the changes work in practice?
Reviewer
The rendered docs for this PR can be found here.
The rendered docs for this PR can be found here.
The rendered docs for this PR can be found here.