OpenNTI / nti.app.site

Other
0 stars 0 forks source link

ICreatedBaseComponents.name is to restrictive #59

Open cutz opened 4 years ago

cutz commented 4 years ago

ICreateBaseComponents.name is currently defined as a DottedName. That means we can't register components for site names such as kibble-1234.nextthought.com. Doing so raises an error such as.

zope.configuration.exceptions.ConfigurationError: Invalid value for 'name'
    File "/home/ntiuser/buildout/etc/package-includes/777-nti.app.analytics.zcml", line 20.1-21.47
    InvalidDottedName: kibble-4224.nextthought.com

I'd like to loosen that restriction. Does anyone know of an existing field or constraint I can pull in to loosen this slightly. This field is used as the name of the BaseComponents and the name of the registered IComponents utility. I can use a TextLine but I then need to add constraints to make sure there is no whitespace, etc. @jamadden @jzuech3 do you know of anything that fits here?

jamadden commented 4 years ago

nti.contentfragments.schema.Tag is a plain text line that can't have spaces but makes no other restrictions.

nti.schema.field.ValidRegularExpression is a TextLine that has to match a regex, that should handle the general case.

cutz commented 4 years ago

Thoughts on using the following regular expression?

[a-zA-Z0-9-_\.]+

jamadden commented 4 years ago

So the intent is upper and lowercase ascii letters, digits, dash, underscore and period? Probably alright, but we should add a restriction that the symbols (and digits?) aren’t first. There may be a way to extend the PythonIdentifier class to make that easier...

Alternately, couldn’t we just normalize the non-identifier chars to, say, underscore? I still like the thought of having valid python paths here, but it’s probably not that important.

cutz commented 4 years ago

So the intent is upper and lowercase ascii letters, digits, dash, underscore and period?

right

Alternately, couldn’t we just normalize the non-identifier chars to, say, underscore? I still like the thought of having valid python paths here, but it’s probably not that important.

Right now we look up the IComponents registered by this name using the incoming hostname of the request. We could get into mappings at lookup time (we have a way to do that) but it seems more straightforward to matches these names to the hostnames (as we usually do). We talked about for trial sites having a -1234 suffix on the subdomain. That is at odds with requiring these be a DottedName or python identifier.

jamadden commented 4 years ago

We talked about for trial sites having a -1234 suffix on the subdomain. That is at odds with requiring these be a DottedName or python identifier.

Right. That (the incoming hostname) was what I was talking about normalizing (I didn't explain that well).

cutz commented 4 years ago

So we could do that dynamically or we could provide a more explicit mapping with the registerSiteMapping directive that we use for vanity urls sometimes.

    <appsite:createBaseComponents bases="nti.appserver.policies.sites.BASEADULT" 
                                  name="kibble.nextthought.com" />

    <appsite:registerInNamedComponents registry="kibble.nextthought.com">

          <!-- snip -->

    </appsite:registerInNamedComponents>

    <sites:registerSiteMapping source_site_name="kibble-4242.nextthought.com"
                target_site_name="kibble.nextthought.com" />

In reality as we get in to letting people change their hostnames it may get confusing if the IComponents name looks like it should be a hostname which no longer matches. Not to mention their old domain shows up in urls (I could imagine people being annoyed by that). Maybe we should make these IComponent names opaque and rely only on the mappings?

jamadden commented 4 years ago

In reality as we get in to letting people change their hostnames it may get confusing if the IComponents name looks like it should be a hostname which no longer matches. Not to mention their old domain shows up in urls (I could imagine people being annoyed by that). Maybe we should make these IComponent names opaque and rely only on the mappings?

Yeah, that may be a good idea. "Convention over configuration" of course, which is how we started, but if there's no actual convention any more, then it just gets confusing with all the special cases. And the way to fix "special cases aren't special enough to break the rules" is to change the rule.