700539 - Mozsvc.config items() method requires a section name parameter.

Status: VERIFIED FIXED

(Cloud Services :: Server: Core, defect)

Description

[Toby Elliott [:telliott]]

This is bad; it means that it doesn't work like a dictionary, and places like the cef lib that expect it to be work that way bail out.

having a section parameter is good and useful, but it should be optional.

Comment 1

[Toby Elliott [:telliott]]

ugh, but of course RawConfigParser expects one

Comment 2

[Ryan Kelly [:rfkelly]]

There was a similar issue in Bug 675105 where the config parser was not sufficiently dict-like.  I don't think it was resolved in the end because there was no consensus that it *should* be dict like.  Should it?

In this case, what values should be returned when no section is specified?  All items?  Items from the "default" section?

There is also a get_map method that might be useful instead.

Comment 3

[Toby Elliott [:telliott]]

Here's the core problem:

http://hg.mozilla.org/services/cef/file/8f7edb791b10/cef.py#l192

we have a ton of places in code that pass self.config into that lib. It does the filtering there just fine. I've fixed it in one file to use self.config.get_map(), but this code is everywhere, it'll be inefficient, and I think we have a semantic mismatch now. I worry this is going to bite us a bunch going forwards

(as a side note, it astonishes me that there's no way to pull a section out of pyramid's configurator)

Comment 4

[Toby Elliott [:telliott]]

It may, of course, be that the answer is to change cef.py to just take the preparsed dictionary of just cef items. Looping in tarek for his thoughts there.

Comment 5

[Rob Miller [:rmiller]]

In server-core, there is a distinction btn the config parser (which is vaguely dict-like, but not really) and the actual config object (which is a subclass of dict that simply adds a couple of extra convenience methods).  I strongly recommend we continue to make this distinction w/ the mozsvc setup, and that we make our config object available throughout the app as an attribute on the registry object, i.e. registry.mozcfg, or something similar.

Comment 6

[Toby Elliott [:telliott]]

I'm fine with an answer like Rob's, but one of the things that's really been driving me nuts working with pyramid has been the inability to easily get hold of a config section dictionary. If that were easier (ideally stick it in pyramid!) I think a lot of my other concerns would go away.

Comment 7

[Rob Miller [:rmiller]]

(In reply to Toby Elliott [:telliott] from comment #6)
> I'm fine with an answer like Rob's, but one of the things that's really been
> driving me nuts working with pyramid has been the inability to easily get
> hold of a config section dictionary. If that were easier (ideally stick it
> in pyramid!) I think a lot of my other concerns would go away.

Indeed.  My band-aid fix is that we stick our config object next to the default one (i.e. registry.mozcfg vs. registry.config), so that at least the pattern for fetching the config would be pyramid-like.  This will buy us some time to hack pyramid so that it allows us to actually replace the default config object w/ our own, then we can revert to just using registry.config.

On further thought, maybe we don't even use registry.mozcfg, maybe we should just overwrite the default config object w/ our own.  Then when pyramid grows the configuration hook we won't even need to change our code.

Comment 8

[Ryan Kelly [:rfkelly]]

Do you mean overwrite registry.settings?  I get an error trying to access request.registry.config.

I'm +1 on overwriting this with something like the Config object from server-core.  Normal pyramid apps would see no difference since it acts like a dictionary at the top level.  Our own apps would know they can call registry.settings.getsection() to access a specific section.

This would also get rid of some of the current ugliness that explicitly duplicates the config into pyramid's standard settings dict.

Comment 9

[Rob Miller [:rmiller]]

(In reply to Ryan Kelly [:rfkelly] from comment #8)
> Do you mean overwrite registry.settings?  I get an error trying to access
> request.registry.config.

Duh, sorry.  Yes, this is what I mean.  We should replace the settings object w/ our own config object, which would be identical but w/ the extra helper methods.

> I'm +1 on overwriting this with something like the Config object from
> server-core.  Normal pyramid apps would see no difference since it acts like
> a dictionary at the top level.  Our own apps would know they can call
> registry.settings.getsection() to access a specific section.

Exactly.

> This would also get rid of some of the current ugliness that explicitly
> duplicates the config into pyramid's standard settings dict.

Right.  We need to watch out b/c there are a few different ways to access the settings object, and if we're not careful we might end up w/ a sitch where one way of access gives you our custom object, while another gives you the plain ol' dict.  Chris McDonough (who happens to be sitting next to me as I type this) is down w/ adding the hooks into pyramid itself, so we won't have to worry about it, but until that's done we should just brute force it.

Comment 10

[Ryan Kelly [:rfkelly]]

Attachment: patch to replace registry.settings with a custom object

Here's a first attempt at this.  SettingsDict provides a couple of helper methods, and the get_configurator function sets up an instance as the regsitry.settings dict.

I haven't copied the section-merging logic from server-core since it seems like we should be able to manage that separately using dict.getsection() and dict.update().

I'm overwriting the settings dict by just going "registry.settings = blah".  The registry has a descriptor which seems to translate this internally into a "registerUtilty(blah, ISettings)".  Are there other ways of getting at the settings that won't be covered by this?

Comment 11

[Rob Miller [:rmiller]]

Comment on attachment 573033 [details] [diff] [review]
patch to replace registry.settings with a custom object

Review of attachment 573033 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.  I think how you've done it will cover all of the settings access cases.  If not, I'm sure we'll find out soon enough... ;)

Comment 12

[Ryan Kelly [:rfkelly]]

Committed in https://github.com/mozilla-services/mozservices/commit/0162e36b45308dda78a1dd3a8f2fe0e925dea1cb

Toby, does this fix the original problem?  Is it now just a matter of passing registry.settings directly into the cef library?

Comment 13

[Ryan Kelly [:rfkelly]]

Passing request.registry.settings into cef_log seems to be working out fine, and the modified settings object has helped clean up some code, so I'm closing this bug.