Configmanized middleware #811
Conversation
| @@ -34,10 +34,20 @@ def __init__(self, *args, **kwargs): | |||
|
|
|||
| """ | |||
| self.context = kwargs.get("config") | |||
| # copy for legacy | |||
There was a problem hiding this comment.
this section of code ought to change to use .../socorro/external/postgresql/connection_context.py If not now, then mark it with a TODO or file a bug for the change.
There was a problem hiding this comment.
I'll do that change as part of my middleware cleanup, see https://bugzilla.mozilla.org/show_bug.cgi?id=778744
There was a problem hiding this comment.
@twobraids I'd rather not mess with it now. This approach makes it so that both the old and new middleware can co-work at the same time. Right, @AdrianGaudebert
|
throughout this middleware refactoring, the logging has not been updated to the "new" style. Rather than fetching the logger from a magic global, we've put ia logging object in the config dictionary under the key "logger". For consistency, I'd like to see that form used in the middleware. |
|
I also like to see some consistent naming applied to the middleware. In the other configmanized apps, the configman dict is called "config". In the middleware, it is called "context", let's make that consistent. |
| @@ -311,6 +318,10 @@ def get_frequency(self, **kwargs): | |||
|
|
|||
| See socorro.lib.search_common.get_parameters() for all filters. | |||
| """ | |||
| # aliases | |||
| kwargs['from_date'] = kwargs.get('from') | |||
There was a problem hiding this comment.
are we using 'get' here just to suppress the KeyError if the key is missing? If those keys are missing, is it really not a fatal error? This question may be more for adrian than for peterbe.
There was a problem hiding this comment.
This is something that was added for backward compatibility and that ought to be removed. The search service accepts from and to as parameters where others services use from_date and to_date. There should be no fatal error if a parameter is missing here as first those are not mandatory parameters and second if it were it would be handled somewhere else (after parameters are filtered).
There was a problem hiding this comment.
@AdrianGaudebert I think the problem is that it says "aliases" in the comment. But in fact, it's really important.
The documentation says "from" and "to" but the code really needs it to be "from_date" and "to_date". https://github.com/peterbe/socorro/blob/bb51fe28cffe1b0e0010f970040a31a2e12bf830/socorro/external/postgresql/base.py#L129
If anything, I should remove the comment # aliases and leave it as is.
@twobraids to answer your question. The code below expects from_date to be defined in kwargs. By using .get() I get either the from date or a None.
|
Is there a bug associated with this pull request? |
|
|
||
| class Bugs(PostgreSQLBase): | ||
| """Implement the /bugs service with PostgreSQL. """ | ||
| filters = [ |
There was a problem hiding this comment.
Why do you move that out of the function? For a small class like Bugs that has only one method it works, but other classes like Crashes can't do that. Is it not better to keep it consistent?
There was a problem hiding this comment.
Ok now I see why you did that. I find it really tricky but it seems to allow us to work around the POST params problem entirely. We will need clear documentation on that though, there's no chance anyone would figure it out without clear explanations imo.
There was a problem hiding this comment.
Yeah, it sucks. It's unfortunately the way the implementations work and I can't think of a better way to solve it.
|
So what are we blocked on here? I'd like to get that, at least, merged to master. Is the revisions problem the only one blocking us now? |
|
@AdrianGaudebert we're blocked on a r+ from @twobraids and you. :) |
|
if it works then r+ |
…d-to-tiers Add method to tiers for optgroups (bugs 882831, 882836)
lars, adrian r?
All the tests for the new middleware_app work and the test coverage is complete.
At this point, some tests of the implementations (e.g unittest/external/postgresq/) are breaking because I'm running a pg 9.1 and there's some problems with CITEXT and mobeta tables.
I chose to delete all the old
socorro/middleware/*files that we don't use any more. I did this because with the recent (albeit small) changes to the implementations, they no longer work.