feat: add dynamic url info by Pablu23 · Pull Request #998 · fkie-cad/Logprep

Pablu23 · 2026-07-22T09:19:16Z

Description

Briefly summarize your changes in a few bullet points (can and should correspond to CHANGELOG.md)
Relates to #XXX (insert issue number here), if there is a corresponding GH issue

Assignee

The changes adhere to the contribution guidelines
I have performed a self-review of my code
My changes generate no new warnings (e.g. flake8/mypy/pytest/...) other than deprecations

Documentation

README.md is up-to-date
CHANGELOG.md is up-to-date
Documentation (doc/source) is up-to-date
Preview for docs looks fine
Notebooks are up-to-date and functional
Examples are up-to-date and functional (including compose & k8s)

Code Quality

Patch test coverage > 95% and does not decrease
New code uses correct & specific type hints

How did you verify that the changes work in practice?

Ran tests

Reviewer

The code is readable/maintainable and follows the contribution guidelines
Documentation is up-to-date
Tests (unit & acceptance) are meaningful and not over-mocked

The rendered docs for this PR can be found here.

mhoff · 2026-07-23T07:42:44Z

+        super().setup()
+        for rule in self.rules:
+            rule = typing.cast(GenericAdderRule, rule)
+            rule.init_generic_adder(self._job_tag_for_cleanup)


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?

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?

Yes we do, I forgot to add the _shut_down function here, this makes it clearer why we need to init here

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.

mhoff · 2026-07-23T08:12:06Z

+            ),
+            converter=lambda x: x if x is None or isinstance(x, list) else [x],
+            default=None,
            eq=False,


Not sure about the precise use case for eq=False... but we would theoretically also need it for add_from_url as well

Not sure about the precise use case for eq=False... but we would theoretically also need it for add_from_url as well

Yeah im unsure about this aswell, would we just want to remove the eq here then?

Maybe remove it and see what happens. I'd figure rule equality might have something to do with the rule tree or the auto rule tester. Might also be a question we could place Monday. I suspect we can remove it, tbh. Either way, remove it completely or add it to both fields - no in between

Maybe remove it and see what happens. I'd figure rule equality might have something to do with the rule tree or the auto rule tester. Might also be a question we could place Monday. I suspect we can remove it, tbh. Either way, remove it completely or add it to both fields - no in between

Okay I asked ai, and it seems like eq=False says dont check this field when comparing two rules against each other. Why we add that here and nowhere else is because, eager loading of a rule file adds the content to add which DOES get checked for equality. Meaning two rules, one with a static add, and one pointing at a file with the same content, are both evaulating to be the same rule. In that case I would actually leave eq=False for add_from_file only so that logic stays, but that doesnt make sense for add_from_url as we cannot eagerly load that.

I understand. Please add a comment on the eq=False to explain this and maybe also add eq=True for add_from_url and explain why this is True

mhoff · 2026-07-23T08:40:09Z

 from logprep.util.defaults import ENV_NAME_LOGPREP_GETTER_CONFIG
 from logprep.util.getter import HttpGetter




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

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

Yes thats right, but I would rather have this be in another PR / task of itself

…rectly

mhoff · 2026-07-24T10:37:16Z

+    def get_list(self, content_field: str | None = None) -> list:
+        """Gets list and fails otherwise"""
+
+        content = self._resolve_content_by_content_type()
+
+        content = Getter._apply_content_field(content, content_field)


Can you add this to get_dict as well? Not related to your PR, but it would make sense to add support to all three high-level interface methods in the same way.

Also, my personal preference would be:

Suggested change

def get_list(self, content_field: str | None = None) -> list:

"""Gets list and fails otherwise"""

content = self._resolve_content_by_content_type()

content = Getter._apply_content_field(content, content_field)

def get_list(self, content_field: str | None = None) -> list:

"""Gets list and fails otherwise"""

content = self._resolve_content_by_content_type()

content = Getter._apply_content_field(content, content_field)

(so without the line break)

Sure ill fix the line breaks, but adding this to get_dict somehow feels wrong to me, because we just have to do this here and for collection because we want to return a list, but in the dict, we can just get the content_field value like you would normally in a dict. Whats your opinion on this?

mhoff · 2026-07-24T10:37:39Z

 """

 import typing
+from typing import Sequence


Suggested change

from typing import Sequence

from collections.abc import Sequence

mhoff · 2026-07-24T10:37:53Z

 """

 import typing
+from typing import Sequence


Suggested change

from typing import Sequence

from collections.abc import Sequence

mhoff · 2026-07-24T10:39:32Z

+    @property
+    def _rules(self) -> Sequence[GenericAdderRule]:
+        """Returns all rules"""
+        return typing.cast(Sequence[GenericAdderRule], self.rules)


Why not "rules"?

Suggested change

@property

def _rules(self) -> Sequence[GenericAdderRule]:

"""Returns all rules"""

return typing.cast(Sequence[GenericAdderRule], self.rules)

@property

def rules(self) -> Sequence[GenericAdderRule]:

"""Returns all rules"""

return typing.cast(Sequence[GenericAdderRule], super().rules)

like in

@property def rules(self) -> Sequence[PseudonymizerRule]: """Returns all rules""" return typing.cast(Sequence[PseudonymizerRule], super().rules)

and other sites

mhoff · 2026-07-24T10:40:55Z

+    url: str = field(
+        validator=[validators.instance_of(str), validators.matches_re(r"^https?://.+")]
+    )
+
+    target_field: str | None = field(
+        default=None, validator=validators.optional(validators.instance_of(str))
+    )
+
+    target_field_mapping: dict[str, str] = field(
+        validator=validators.deep_mapping(
+            key_validator=validators.instance_of(str),
+            value_validator=validators.instance_of(str),
+        ),
+        factory=dict,
+    )


Missing pydoc for all three

mhoff · 2026-07-24T10:44:48Z

+    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")


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.

Suggested change

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")

mhoff · 2026-07-24T10:45:09Z

+            default=None,
+            validator=validators.optional(validators.instance_of(AddFromUrlConfig)),
+            converter=_convert_add_from_url,
+        )


pydoc missing

mhoff · 2026-07-24T10:45:31Z

@@ -160,20 +213,47 @@ def _refresh_add(self):
        def __attrs_post_init__(self):


super needs to be called here

mhoff · 2026-07-24T10:46:40Z

+                    # 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)


We have the job_tag now and can resolve this properly

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?

mhoff · 2026-07-24T10:47:09Z

+                )
+
+            # Eagerly loaded from file
+            for add_file in self.add_from_file:  # pylint: disable=not-an-iterable


Suggested change

for add_file in self.add_from_file: # pylint: disable=not-an-iterable

for add_file in self.add_from_file:

It's an iterable, right?

mhoff · 2026-07-24T10:48:24Z

+        self._dynamic_template = resolved_template
+        self._dynamic_identifiers = tuple(resolved_template.get_identifiers())
+
+        if len(self._dynamic_identifiers) > 0:


Suggested change

if len(self._dynamic_identifiers) > 0:

if self._dynamic_identifiers:

You could even go all fancy with

Suggested change

if len(self._dynamic_identifiers) > 0:

self._is_dynamic = bool(self._dynamic_identifiers)

but I think the if makes sense.

However, you can simply merge this if with the check afterwards

mhoff · 2026-07-24T10:50:53Z

+
+            self._static_uri = static_uri
+
+    def _dynamic_add_from_url(self, event: dict) -> dict[str, FieldValue]:


Typing missing on dict

mhoff · 2026-07-24T10:53:36Z

+            self._static_uri = static_uri
+
+    def _dynamic_add_from_url(self, event: dict) -> dict[str, FieldValue]:
+        config = typing.cast(GenericAdderRule.Config, self._config)


Use the standard casted config property for this

mhoff · 2026-07-24T10:54:16Z

+
+    def add(self, event: dict) -> dict:
        """Returns the fields to add"""
        config = typing.cast(GenericAdderRule.Config, self._config)


Same here, casted property

mhoff · 2026-07-24T10:56:35Z

+        if config.add_from_file or config.add:
+            return config.add


Is this the logic?

if config.add_from_file => config.add should already be populated through attrs_post_init

if config.add => config.add already preloaded through attrs_post_init or static http

Shouldn't the check then be simply if config.add?

Nevermind, I confused config.add with self.add. But you want to return self.add here, right?

You are correct, I initially didnt understand that add_from_file populates add automatically

mhoff · 2026-07-24T11:00:38Z

+        if not self._is_dynamic:
+            assert self._static_uri
+            return self._content_to_items_to_add(self._dynamic_content[self._static_uri])


This reads weird.

if not dynamic: ... get dynamic content ...

Yes you are right, I might have to change the name of _dynamic_content, as that is just a cache

mhoff · 2026-07-24T11:01:46Z

+    def _content_to_items_to_add(self, content: FieldValue):
+        items_to_add: dict[str, FieldValue] = {}
+
+        config = typing.cast(GenericAdderRule.Config, self._config)


mhoff · 2026-07-24T11:03:02Z

+            assert config.add_from_url.target_field_mapping is not None
+
+            if not isinstance(content, dict):
+                raise ValueError("add_from_url.target_field_mapping requires a mapping response")


Suggested change

raise ValueError("add_from_url.target_field_mapping requires a mapping response")

raise ValueError(f"add_from_url.target_field_mapping requires a mapping response, got: {content}")

mhoff · 2026-07-24T11:03:57Z

+                item = get_dotted_field_value_with_missing(content, mapping_source_field)
+                if item is MISSING:
+                    logger.warning(
+                        "could not add source_field: %s for target_field: %s because was missing",


Suggested change

"could not add source_field: %s for target_field: %s because was missing",

"could not add source_field %s for target_field %s because it is missing",

I think it reads weird with the colons. Rather "source_field=%s" or "source_field %s" or "source_field '%s'" I'd say

mhoff · 2026-07-24T11:19:26Z

+                        mapping_source_field,
+                        mapping_target_field,
+                    )
+                    continue


If we stick to this, this might be a case for a processor warning

Pablu23 self-assigned this Jul 22, 2026

Pablu23 changed the title ~~Feat add dynamic url info~~ feat: add dynamic url info Jul 22, 2026

mhoff reviewed Jul 22, 2026

View reviewed changes

Comment thread logprep/abc/getter.py Outdated

mhoff reviewed Jul 22, 2026

View reviewed changes

Comment thread logprep/processor/generic_adder/processor.py Outdated