Changed existing fqname methods.

Changed existing fqname methods.

[Thomas.J.Waldmann, 2013-07-10 15:23:49]

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/converter/nonexist...
File MoinMoin/converter/nonexistent_in.py (right):

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/converter/nonexist...
MoinMoin/converter/nonexistent_in.py:28: item_name = rev.item.fqname.value
are you just assuming that the field is NAME_EXACT? how can you be sure?

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/storage/middleware...
File MoinMoin/storage/middleware/indexing.py (right):

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/storage/middleware...
MoinMoin/storage/middleware/indexing.py:123: names = []
well, either you keep it like it was.

or, if you made sure that the comment in 121/122 does not apply any more, you
remove the comment also.

currently it just doesn't fit together any more (and you didn't read your
changeset otherwise you would also have noticed that).

I am rather unhappy about you not doing that after being told that multiple
times. :(

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/storage/middleware...
MoinMoin/storage/middleware/indexing.py:919: assert isinstance(name, unicode) or
not name
why did you change this line?

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/storage/middleware...
MoinMoin/storage/middleware/indexing.py:927: return
CompositeName(self.namespace, u'', name)
hmm, u'' or NAME_EXACT?

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/storage/middleware...
MoinMoin/storage/middleware/indexing.py:1183: return
CompositeName(self.namespace, u'', name)
hmm, u'' or NAME_EXACT?

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/storage/middleware...
File MoinMoin/storage/middleware/protecting.py (right):

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/storage/middleware...
MoinMoin/storage/middleware/protecting.py:90: itemname = fqname.value if
fqname.field == u'' else u''
field u'' or NAME_EXACT?

https://codereview.appspot.com/10869045/diff/7001/MoinMoin/storage/middleware...
MoinMoin/storage/middleware/protecting.py:114: q = {field: fqname.value}
maybe fqname class could have a method .query() just returning that dict.

[Thomas.J.Waldmann, 2013-07-15 15:33:49]

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/converter/nonexis...
File MoinMoin/converter/nonexistent_in.py (right):

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/converter/nonexis...
MoinMoin/converter/nonexistent_in.py:30: xlink.href: Iri(scheme='wiki',
authority='', path='/' + item_name, query='do=modify'),
can the other code cope with what you put into path here?

it used to be a simple name, but now may have namespaces, fields, non-name
values

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
File MoinMoin/storage/middleware/indexing.py (right):

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:183: return self.fqname.value
you just return the value without knowing whether it really is a name?

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:859: return self._current.get(NAMESPACE)
or u''
shouldn't the NAMESPACE be always there?

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:920: def _fqname(self, name):
code below looks a bit like name argument is optional.

declare it with name=None?

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:924: if name:
if name is not None:

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:1178: return self.meta.get(NAMESPACE) or
u''
see above

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:1204: return [self.fqname]
that all looks duplicated from above

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
File MoinMoin/storage/middleware/protecting.py (right):

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/protecting.py:97: raise ValueError('No acl_mapping
entry found for item {0!r}'.format(fqname))
let's assume some specific item with name "foo" has a specific itemid 12345.

if we access it via name, a configured acl for "foo" will get applied.

if we access it via @itemid/12345 it will apply acl configured for "".

thus we have different acls depending on how one accesses items - not good.

this function needs to get a list of all names of the item and determine the
acl_cfg based on that:

for prefix, acls in self.acl_mapping:
    if any itemname startswith prefix:
         return acls

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/protecting.py:123: acl_cfg =
self._get_configured_acls(fqname)
caller needs to get changed accordingly, see above.

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/protecting.py:197: # if we get a list of names, just
use first one to fetch item
the comment does not match the "if" above.

nor does the code below deal with a unicode object in a sane way.

please check how a clean api here should be like.

[ashu1461, 2013-07-16 05:17:30]

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/converter/nonexis...
File MoinMoin/converter/nonexistent_in.py (right):

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/converter/nonexis...
MoinMoin/converter/nonexistent_in.py:30: xlink.href: Iri(scheme='wiki',
authority='', path='/' + item_name, query='do=modify'),
will deal with it separately, 
after understanding how the other code works..

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
File MoinMoin/storage/middleware/protecting.py (right):

https://codereview.appspot.com/10869045/diff/24002/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/protecting.py:97: raise ValueError('No acl_mapping
entry found for item {0!r}'.format(fqname))
umm ..
How function fqname (in indexing.py) works is if item.name(say "foo") exists
(doesn't matter how we call it/access it through name/itemid) then it returns
fqname = CompositeName(namespace, NAME_EXACT, foo) 
and if no name exists then it returns CompositeName(namespace, ITEMID,
item.itemid).

So we wont be having this problem.

[ashu1461, 2013-07-16 17:36:59]

https://codereview.appspot.com/10869045/diff/42001/MoinMoin/storage/middlewar...
File MoinMoin/storage/middleware/indexing.py (right):

https://codereview.appspot.com/10869045/diff/42001/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:840: return self.meta.get(NAMESPACE) or
u''
It is not compulsory to have namespace field in the meta.

[Thomas.J.Waldmann, 2013-07-16 19:40:04]

https://codereview.appspot.com/10869045/diff/42001/MoinMoin/storage/middlewar...
File MoinMoin/storage/middleware/indexing.py (right):

https://codereview.appspot.com/10869045/diff/42001/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:183: return self.fqname.value if
self.fqname.field == NAME_EXACT else get_names(self.meta)[0]
where does self.meta come from?

https://codereview.appspot.com/10869045/diff/42001/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:823: self.meta = meta
where is this called from?

think about whether using super() or a Mixin class is better for this purpose.

[ashu1461, 2013-07-16 21:22:28]

https://codereview.appspot.com/10869045/diff/42001/MoinMoin/storage/middlewar...
File MoinMoin/storage/middleware/indexing.py (right):

https://codereview.appspot.com/10869045/diff/42001/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/indexing.py:183: return self.fqname.value if
self.fqname.field == NAME_EXACT else get_names(self.meta)[0]
no need of this method :P
not getting called from anywhere.

[Thomas.J.Waldmann, 2013-07-17 22:48:44]

ok