Hey @Abhijeet17o ! I really appreciate your effort, but I would prefer if we could keep this PR focused on the two improvements that you proposed in the issue. Right now, it is hard for me to see the purpose of the single code changes. I am sure that all your optimizations benefit performance, but they also add a lot of additional code (=complexity) and I am not sure if that is worth it.

Hey @regulartim, you are right to point it out that I have kind of went beyond my scope. The PR grew beyond its original scope because I incorporated several automated code review suggestions that each seemed reasonable in isolation, but together added complexity that wasn't necessary for the two fixes I proposed.
The extra optimizations were real improvements, but they belong in separate focused PRs
So, right now I'm stripping it back to just the two changes from the issue.

Hey @regulartim!
Can you please review the changes made.
I've narrowed the focus on problems that I had mentioned in the issue.

Hey @regulartim,
I wanted to solve one question that I had. So in the following code:

honeypot_set = {hp.name for hp in ioc.general_honeypot.all()}
if honeypot_name not in honeypot_set: 
    honeypot = self._honeypot_cache.get(self._normalize_name(honeypot_name))
    if honeypot is not None:
        ioc.general_honeypot.add(honeypot)  # this fires an extra query

if honeypot_name comes in as "cowrie" but the DB stores "Cowrie", the guard (not in honeypot_set) doesn't recognize it as already associated, so it falls through to the [add()]. The cache lookup finds the object (because it uses _normalize_name), and Django's M2M add() is idempotent so no duplicate is created but an unnecessary DB query fires

Can you validate this? if yes I'll implement a solution for it in this PR itself

EDIT: Implemented the requited changes aswell 👍

Hey @regulartim ! I've already stripped things back down, the current diff is only 20 lines of production code that is in ioc.py, it is working as intended. This is all tied to two fixes:

• Fix 1: one line adding prefetch_related("general_honeypot") to get_ioc_by_name()
• Fix 2: three lines changing _honeypot_cache to store objects instead of booleans, and updating is_enabled() and create_honeypot() accordingly
• The error log in add_honeypot_to_ioc() was added at the suggestion that you made ("I guess we should log an error then")

The major code addition was in the test_ioc_repository.py‎ where tests for the modifications were added and documented.
So, actually I would want some guidance on would you like me to remove any specific tests you feel are excessive? Or how would you like me to approach it?

Hey @regulartim,
I wanted to solve one question that I had. So in the following code:
honeypot_set = {hp.name for hp in ioc.general_honeypot.all()}
if honeypot_name not in honeypot_set:
honeypot = self._honeypot_cache.get(self._normalize_name(honeypot_name))
if honeypot is not None:
ioc.general_honeypot.add(honeypot) # this fires an extra query
if honeypot_name comes in as "cowrie" but the DB stores "Cowrie", the guard (not in honeypot_set) doesn't recognize it as already associated, so it falls through to the [add()]. The cache lookup finds the object (because it uses _normalize_name), and Django's M2M add() is idempotent so no duplicate is created but an unnecessary DB query fires
Can you validate this? if yes I'll implement a solution for it in this PR itself
EDIT: Implemented the requited changes aswell 👍

@Abhijeet17o You added this additional logic, which is not one of the fixes you proposed in the issue. Or am I wrong?

@regulartim
Oh, I think now I actually get your point. So test cases addition is fine but the modifications that are made to the non-test scripts should strictly be related to what has been defined in the issue.
Correct me if I'm wrong

I'll do the needful now, thanks! 👍

Hey @regulartim,
I have stripped things down now, and I don't think so there is anything that is out of scope for this issue. Can you review it once, thanks!