Re: Trouble with new tracker designator?

John,

Thank you for your well thought-out response. Also I didn't check, I'm pretty sure that 'zip2py' would fail looking for object ID 2 of class zip.

According to a google search, a Python identifier (e.g. Python class name) starts with A-Za-z or _, and is followed by 0 or more A-Za-z0-9_. For roundup class names that changes since it doesn't end with a digit. Likewise a designator is a class name followed by one or more digits (see the roundup-admin help for retire). Here is a short unittest program that tests various cases using re:

#!/usr/bin/env python3
"""
Tests for roundup designators and class
"""

import re
import unittest

class_re = r'^([A-Za-z_](?:[A-Za-z_0-9]*[A-Za-z_]+)?)$'
designator_re = r'^([A-Za-z_](?:[A-Za-z_0-9]*[A-Za-z_]+)?)(\d+)$'

TEST_CASES = [('zip2py44', 'designator', ('zip2py', '44')),
         ('zip2py', 'class', ('zip2py',)),
         ('zippy2', 'designator', ('zippy', '2')),
         ('a9', 'designator', ('a', '9')),
         ('a1234', 'designator', ('a', '1234')),
         ('a', 'class', ('a',)),
         ('_', 'class', ('_',)),
         ]

class TestNames(unittest.TestCase):
    def test_all(self):
        for (name, which, expected) in TEST_CASES:
            m_class = re.match(class_re, name)
            m_designator = re.match(designator_re, name)
            if which == 'class' and  m_class is not None and m_designator is None:
                got = m_class.groups()
                self.assertEqual(got, expected)
            elif which == 'designator' and m_class is None and m_designator is not None:
                got = m_designator.groups()
                self.assertEqual(got, expected)
            else:
                self.fail('name=%s, which=%s' % (name, which))

if __name__ == '__main__':
    unittest.main()

I think I caught all of the cases. Feel free to add more.

If splitDesignator in hyperdb.py were changed to use designator_re (above) it would do a better job of detecting valid and invalid designators. Note that that function uses re.match, which only has an anchor on the first character of the string. Likewise, change the constructor (__init__) method for hyperdb.Class (roundup/hyperdb.py line 901). Something like:

 if not re.match(r'^([A-Za-z_](?:[A-Za-z_0-9]*[A-Za-z_]+)?)$', classname):
    raise ValueError('Class name %s is not valid. It must start with a letter and not end with a digit.' % (classname,))

Maybe define a function which validates a class name, and another function to validate a designator, to make it easier to write a test case.

I changed my class name to zippytwo to avoid the whole issue.

Tom Ekberg
Senior Computer Specialist,
Department of Laboratory Medicine and Pathology
4th Floor, Pat Steel Building, currently WFH
Home: (253) 561-2509
Email: [email protected]

________________________________
From: John Rouillard <[email protected]>
Sent: Monday, July 20, 2020 1:11 PM
To: Tom Ekberg <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: [Roundup-users] Trouble with new tracker designator?

Hi Tom:

On Mon, Jul 20, 2020 at 12:28 PM Tom Ekberg <[email protected]> wrote:
> I created a new tracker and got the issue.index.html and issue.item.html pages to work properly. I'm having
> trouble with the issue.search.html page. It is at the point where I'm defining a property to search on. The
> HTML code looks like this:
>
> <tr tal:define="name string:zippy2;
>                 db_klass string:zippy2;
>                 db_content string:name;">
>   <th i18n:translate="">Zippy2:</th>
> [...]
> Note this is for a link called zippy2. The schema.py file contains these lines related to zippy2:
>
> zippy2 = Class(db, "zippy2",
>                 name=String(),
>                 order=Number())
> zippy2.setkey("name")
> zippy2=Link("zippy2"),
>
> The last line is in the list of issue properties (IssueClass). I looked at the roundup design document
> (http://roundup.sourceforge.net/docs/design.html#property-names-and-types) in the Property Names and
> Types section. All it says about property names is that they must start with a letter. I suspect that the real
> problem is with the class name that ends with a digit. I saw nothing in that document defining restrictions
> on class names.

I think you are right. The classname looks like a designator for the
zippy class. Hence internally it's being parse
into item 2 of the zippy class. Also I am not sure how the code would
handle the designator "zippy222" I think
the regexps assume the run of numbers at the end is always extracted
as the item number (my guess
was wrong, see below).

Can you see if changing the classname to zip2py works (I suspect it
will fail with no class zip). If it fails
we have the restriction that classnames start with an alpha and no
numbers (but including _). The intent
IIUC was to have the classnames be the same as identifiers in python language.

Looking at hyperdb.py:

  def splitDesignator(designator, dre=re.compile(r'([^\d]+)(\d+)')):
      """ Take a foo123 and return ('foo', 123)
      """

looks like numbers aren't allowed anywhere in the classname. Even _ or
a control character is allowed as first
character if I read that re right. We could allow internal numbers by
changing the dre to:

    r'^(\w+)(\d+)$'

This would allow an initial number, but we could stop that by a check
in the Class init() method as you
recommend below. I am not sure why the re is not anchored given the
specific requirements of a designator.
Grepping the source I don't see any calls to splitDesignator that set dre.

Bern, Ralf any ideas here?

> If you like to read gory details, please read on. The suggested fix is at the end. If not, please tell me what
> you think about what I have said so far.
[...]
> I suggest 2 fixes (sorry for the pun):
>
> 1. Define a section in the design document referenced above that states restrictions on class names, something like:
>
> Class names must start with a letter and cannot end with a digit.

If my guess above about zip2py is right, this needs to be more restrictive.

> 2. Change the constructor (__init__) method for hyperdb.Class (roundup/hyperdb.py line 901). Above that
> line is a check for reserved property names. Just above line 901 I suggest a check for a valid class name.
> Something like:
>
> if not classname[0].isalpha() or classname[-1].isdigit():
>
> raise ValueError('Class name %s is not valid. It must start with a letter and not end with a digit.' % (classname,))

That's the right place to validate the name. We just have to validate
the name correctly once we specify the requirements.

I'm surprised nobody brought this up before. Nice find I guess 8-).

-- rouilj
(responding from my backup email account)

_______________________________________________
Roundup-users mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/roundup-users