Validation for comp name

Validation for comp name

[Thomas.J.Waldmann, 2013-07-10 14:46:41]

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/_tests/test_forms.py
File MoinMoin/_tests/test_forms.py (right):

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/_tests/test_forms....
MoinMoin/_tests/test_forms.py:55: def test_validjson():
what is the primary intention of this test?

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode87
MoinMoin/forms.py:87: There should be not item existing with the same name.
this docstring will likely get out of sync with the code below, so maybe rather
document the code below in a sane way.

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode93
MoinMoin/forms.py:93: return True
how does the code fit to the comment in line 91?

it somehow (again) looks to me like I am the first one reading through the
code...

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode97
MoinMoin/forms.py:97: invalid_names = [name for name in names if
name.startswith('@')]
+ is used by views and should be disallowed also. think about how to efficiently
check for both.

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode101
MoinMoin/forms.py:101: namespaces = [namespace.rstrip('/') for namespace, _ in
app.cfg.namespace_mapping if namespace != current_namespace]
why is the current namespace not in the list?

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode102
MoinMoin/forms.py:102: invalid_names = [name for name in names if
name.split('/', 1)[0] in namespaces]
do names from the name list still have the namespaces prefixed although there is
a separate field for the namespace?

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode104
MoinMoin/forms.py:104: invalid_names.append(fqname.namespace)
?

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode109
MoinMoin/forms.py:109: if current_namespace is not None:
can it be None? what is the case if it is?

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode110
MoinMoin/forms.py:110: query = And([query, Term(NAMESPACE,
meta.get(NAMESPACE))])
why do you .get it again if you already have it in current_namespace?

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode112
MoinMoin/forms.py:112: query = And([query, Not(Term(ITEMID, itemid))])
add a comment about what your are doing here / why.

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode121
MoinMoin/forms.py:121: return True
ok, so this now has 2 behaviours:

return True
raise exception

so, does returning True have any meaning other than not throwing an exception?

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/storage/middleware...
File MoinMoin/storage/middleware/validation.py (right):

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/storage/middleware...
MoinMoin/storage/middleware/validation.py:66: itemid = state[keys.FQNAME].value
you set it in line 64 and in 66 you overwrite it - why?

[ashu1461, 2013-07-10 20:56:26]

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode97
MoinMoin/forms.py:97: invalid_names = [name for name in names if
name.startswith('@')]
On 2013/07/10 14:46:41, Thomas.J.Waldmann wrote:
> + is used by views and should be disallowed also. think about how to
efficiently
> check for both.

done

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode101
MoinMoin/forms.py:101: namespaces = [namespace.rstrip('/') for namespace, _ in
app.cfg.namespace_mapping if namespace != current_namespace]
On 2013/07/10 14:46:41, Thomas.J.Waldmann wrote:
> why is the current namespace not in the list?

I don't remember why i did this, tried hard but could not recall...

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode102
MoinMoin/forms.py:102: invalid_names = [name for name in names if
name.split('/', 1)[0] in namespaces]
On 2013/07/10 14:46:41, Thomas.J.Waldmann wrote:
> do names from the name list still have the namespaces prefixed although there
is
> a separate field for the namespace?

No this is done so as to separate sub-items prefixes and names.

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode104
MoinMoin/forms.py:104: invalid_names.append(fqname.namespace)
On 2013/07/10 14:46:41, Thomas.J.Waldmann wrote:
> ?
Removed.

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode109
MoinMoin/forms.py:109: if current_namespace is not None:
On 2013/07/10 14:46:41, Thomas.J.Waldmann wrote:
> can it be None? what is the case if it is?

Fixed, now it cannot be None ..

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode112
MoinMoin/forms.py:112: query = And([query, Not(Term(ITEMID, itemid))])
On 2013/07/10 14:46:41, Thomas.J.Waldmann wrote:
> add a comment about what your are doing here / why.

Done.

https://codereview.appspot.com/10842043/diff/5001/MoinMoin/forms.py#newcode121
MoinMoin/forms.py:121: return True
On 2013/07/10 14:46:41, Thomas.J.Waldmann wrote:
> ok, so this now has 2 behaviours:
> 
> return True
> raise exception
> 
> so, does returning True have any meaning other than not throwing an exception?

No, it doesnot i removed the return statement here, and we have a empty return
above.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode88
MoinMoin/forms.py:88: return
What is the problem, if names = [] is None return true, that is what the comment
says ... names = [] is valid

[ashu1461, 2013-07-10 21:17:39]

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms.py
File MoinMoin/_tests/test_forms.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:55: def test_validjson():
Main intention: To make sure that the name/itemid validators are working
properly.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/storage/middlewar...
File MoinMoin/storage/middleware/validation.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/validation.py:66: itemid = state[keys.FQNAME].value
On 2013/07/10 14:46:41, Thomas.J.Waldmann wrote:
> you set it in line 64 and in 66 you overwrite it - why?

the first one is the original itemid(can be none or the previous itemid), if the
field is ITEMID, then we would like itemid to be the fqname value (which can be
different from itemid obtained above).

[Thomas.J.Waldmann, 2013-07-11 22:17:08]

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms.py
File MoinMoin/_tests/test_forms.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:55: def test_validjson():
On 2013/07/10 21:17:39, ashu1461 wrote:
> Main intention: To make sure that the name/itemid validators are working
> properly.

somehow the test looks far too complicated.

can you test the name and itemid validators by simpler tests?

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:64: # ('names','namespace', 'field', 'value',
'result'),
please have the comment on top of the stuff

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:67: ([u'ns1ns2ns3', u'ns1/subitem'], '', '',
'valid', False),
why False?

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:71: ([u'validname'], 'ns1/ns2', '', 'validname',
False),
why False?

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:72: ([u'validname'], '', '', 'validname', not
flaskg.storage[u'validname'] is None),
?

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode85
MoinMoin/forms.py:85: current_namespace = meta.get(NAMESPACE) or
fqname.namespace
that doesn't look logically consistent

e.g. if meta[NAMESPACE] == "ns", if will use "ns"
if meta[NAMESPACE] == "" (which is the default ns), it will use fqname.namespace

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode88
MoinMoin/forms.py:88: return
well, maybe the main problem is that the behaviour (regarding return value and
exception usage) of this function is undocumented.

"validname" btw is suggesting it is returning a boolean value.
if it is just validating and returning or raising an exception, it should be
"validate_name"

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode91
MoinMoin/forms.py:91: # Item name cannot start with '@' or '+', '@something'
denotes a field where as '+something' deotes a view.
... must not ... denotes ...

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode92
MoinMoin/forms.py:92: invalid_names = [name for name in names if
name.startswith(('@', '+'))]
good :)

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode95
MoinMoin/forms.py:95: # Item names should not match with existing namespaces.
must not

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode101
MoinMoin/forms.py:101: #There should be not item existing with the same name.
# ...

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode108
MoinMoin/forms.py:108: duplicate_names |= set([name for name in result[NAME] if
name in names])
you could use a set intersection on the right side.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/storage/middlewar...
File MoinMoin/storage/middleware/validation.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/validation.py:66: itemid = state[keys.FQNAME].value
reduce code duplication in the 2 lines above

[ashu1461, 2013-07-11 22:50:17]

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms.py
File MoinMoin/_tests/test_forms.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:55: def test_validjson():
On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> On 2013/07/10 21:17:39, ashu1461 wrote:
> > Main intention: To make sure that the name/itemid validators are working
> > properly.
> 
> somehow the test looks far too complicated.
> 
> can you test the name and itemid validators by simpler tests?

I will try ..

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:64: # ('names','namespace', 'field', 'value',
'result'),
On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> please have the comment on top of the stuff

There is a pep8 error if I place it in 63 line ..
i will place it after the 62 line.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:67: ([u'ns1ns2ns3', u'ns1/subitem'], '', '',
'valid', False),
On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> why False?

because of u'ns1/subitem', ns1 is an existing namespace.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:71: ([u'validname'], 'ns1/ns2', '', 'validname',
False),
On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> why False?
I created an item named validname in namespace ns1/ns2 above 57-60.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:72: ([u'validname'], '', '', 'validname', not
flaskg.storage[u'validname'] is None),
On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> ?
You don't know whether an item named 'validname' exists or not. 
so have to find it through a whoosh query.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode85
MoinMoin/forms.py:85: current_namespace = meta.get(NAMESPACE) or
fqname.namespace
On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> that doesn't look logically consistent
> 
> e.g. if meta[NAMESPACE] == "ns", if will use "ns"
> if meta[NAMESPACE] == "" (which is the default ns), it will use
fqname.namespace

I thought it will function the other way .. 
if meta.get(NAMESPACE) is None:
    current_namespace = fqname.namespace

I will fix this.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode88
MoinMoin/forms.py:88: return
On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> well, maybe the main problem is that the behaviour (regarding return value and
> exception usage) of this function is undocumented.
> 
> "validname" btw is suggesting it is returning a boolean value.
> if it is just validating and returning or raising an exception, it should be
> "validate_name"

fine.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/storage/middlewar...
File MoinMoin/storage/middleware/validation.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/storage/middlewar...
MoinMoin/storage/middleware/validation.py:66: itemid = state[keys.FQNAME].value
On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> reduce code duplication in the 2 lines above

ok.

[Thomas.J.Waldmann, 2013-07-11 23:01:46]

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms.py
File MoinMoin/_tests/test_forms.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:64: # ('names','namespace', 'field', 'value',
'result'),
On 2013/07/11 22:50:17, ashu1461 wrote:
> On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> > please have the comment on top of the stuff
> 
> There is a pep8 error if I place it in 63 line ..
> i will place it after the 62 line.

ok

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:67: ([u'ns1ns2ns3', u'ns1/subitem'], '', '',
'valid', False),
On 2013/07/11 22:50:17, ashu1461 wrote:
> On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> > why False?
> 
> because of u'ns1/subitem', ns1 is an existing namespace.

ah, right. and the names do NOT contain the namespace.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:71: ([u'validname'], 'ns1/ns2', '', 'validname',
False),
On 2013/07/11 22:50:17, ashu1461 wrote:
> On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> > why False?
> I created an item named validname in namespace ns1/ns2 above 57-60.
> 

use "existingname" to make it clearer?

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:72: ([u'validname'], '', '', 'validname', not
flaskg.storage[u'validname'] is None),
On 2013/07/11 22:50:17, ashu1461 wrote:
> On 2013/07/11 22:17:09, Thomas.J.Waldmann wrote:
> > ?
> You don't know whether an item named 'validname' exists or not. 
> so have to find it through a whoosh query.

we initialize the backends rather often to make tests independant.
maybe avoid naming conflicts in the same test module, though.

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

https://codereview.appspot.com/10842043/diff/15001/MoinMoin/forms.py#newcode85
MoinMoin/forms.py:85: current_namespace = meta.get(NAMESPACE) or
fqname.namespace
if you just want a default, use the 2nd arg to .get()

[Thomas.J.Waldmann, 2013-07-13 15:01:44]

https://codereview.appspot.com/10842043/diff/28002/MoinMoin/_tests/test_forms.py
File MoinMoin/_tests/test_forms.py (right):

https://codereview.appspot.com/10842043/diff/28002/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:63: # ('names','namespace', 'field', 'value',
'result')
insert the missing blank

https://codereview.appspot.com/10842043/diff/28002/MoinMoin/forms.py
File MoinMoin/forms.py (right):

https://codereview.appspot.com/10842043/diff/28002/MoinMoin/forms.py#newcode82
MoinMoin/forms.py:82: Checks wether the name is valid or not.
check whether the names are valid.

Will just return, if they are valid, will raise a NameNotValidError if not.

https://codereview.appspot.com/10842043/diff/28002/MoinMoin/forms.py#newcode88
MoinMoin/forms.py:88: return
the comment in 86 still does not match the code below (in 87/88) and is rather
confusing the reader than helping.

https://codereview.appspot.com/10842043/diff/28002/MoinMoin/forms.py#newcode91
MoinMoin/forms.py:91: # Item names must not start with '@' or '+', '@something'
denotes a field where as '+something' deotes a view.
you did not fix all I pointed out.

[Thomas.J.Waldmann, 2013-07-13 23:29:44]

minor issue still, otherwise fine

https://codereview.appspot.com/10842043/diff/28002/MoinMoin/_tests/test_forms.py
File MoinMoin/_tests/test_forms.py (right):

https://codereview.appspot.com/10842043/diff/28002/MoinMoin/_tests/test_forms...
MoinMoin/_tests/test_forms.py:63: # ('names','namespace', 'field', 'value',
'result')
On 2013/07/13 15:01:45, Thomas.J.Waldmann wrote:
> insert the missing blank

i meant after 'names',