Readability Refactoring _api #50
Conversation
move simple static functions from _api to parsing_utils, check that cleaned payload isn't empty before creating attachment
mapping the request path to the respective resource is a separate step now to make intention clearer
pulled up action parsing to the outermost create_events method. If action is (intentionally) mapped to 'null' the rest of event generation can be omitted.
extracted method for initiator generation, removed action parsing and check for 'null' actions because that happens upstream now
Basically the only attributes of events that are parsed differently based on config are targets, attachments and custom attributes. With this commit, they are parsed beforehand and passed to event generation explicitely
renaming things and removing unused variables
taxonomy.ACTION_LIST caused key error in _key_action_suffix_map
removed _attach_payload in parsing_utils since attach_payload is used
replaced indexing [] with get and default value
|
It's late for me to comment on the content, but I fully support a readability refactoring. It's really challenging to follow the chain of functions for what is a fairly simple concept. I will check on the specifics of the changes at another time. |
|
Is there anything I shall explain or improve? |
Why does the _method_action_map go into utils? If the idea is to cut out simple utils, I'm not sure why the map which contains a specific mapping we rely on moves to utils. |
|
I thought (and I might be wrong here since I don't know how it evolved) this mapping is obvious and it distracted a bit from understanding the handling of requests. However, surely it bears actual semantics so 'util' might not be the perfect classification for it |
|
I don't consider it obvious, a lot of the core breaking points and mappings occur in the pycadf library. The more places mappings, and other data is, the harder it is to sort through. This library is a fork of the openstack keystone middleware, and we do still take changes from it. I'm hesitant to refactor in general due to this, although our version has a ton of changes already from the base version. I really dislike the logic paths of the audit middleware, so I think it's reasonable, but utils in this case should only be domainless methods. Nothing to do with auditing, but simple actions as you've approached it. Having to find a mapping which is quite important in a utils file doesn't work in my mind. |
|
I see your point. I moved it (and the corresponding get_action_from_method) back to _api |
|
The other thing I have is that pep8 is failing, and is required. It's I think all just too long of a line, since pep8 is rather strict with it. You can see it on the build failing if you select it. if you can clean that up, I think I can test and merge it. |
|
Anything else? |
|
I'm on vacation, when I return it's on the list, but the system that this works with has a current upgrade in progress, and that will be done before this gets merged. Apologies for the delays. |
|
Wish you happy holidays! Would be cool if this got merged next year |
The event parsing logic in the _api module consisted of a chaining many, similarly named methods passing through all intermediate results of parsing. Therefore it was not obvious, which part of requests/responses is evaluated for which information/attribute in the final events. The present refactoring aims to improve on that by 1) making path and target parsing explicit (also in the naming of methods) 2) factoring out the extraction of attachments/payloads to make circumstances of those steps more obvious 3) moving the action parsing to the beginning to bypass further processing for suppressed events and 4) simplifying (semantics preserving) the logic, avoiding try-catch and separating static helper functions
This refactoring resulted from my aim to understand and modify this middleware for a personal project and I'm happy to except feedback on it.