web2py / pydal

A pure Python Database Abstraction Layer
BSD 3-Clause "New" or "Revised" License
498 stars 140 forks source link

decimal on GAE #302

Open mdipierro opened 9 years ago

mdipierro commented 9 years ago

https://groups.google.com/forum/#!topic/web2py/EZ37Ipakiaw

stephenrauch commented 9 years ago

@mdipierro

I was unable to duplicate the problem in pydal, but on examination I think the problem is in gluon/validators.py. The stack trace attached above indicates:

  File "C:\Users\David\Google Drive\My Documents\ocsnedb50\gluon\validators.py", line 572, in build_set

    records = self.dbset(table).select(*fields, **dd)

Line 572 is in an 'if' block that uses the private attribute '_dbname' to determine if gae specific code should be run, the test evidently fails, because the non-gae code is being run:

564: if self.dbset.db._dbname != 'gae':
    ...

572:     records = self.dbset(table).select(*fields, **dd)
573: else:
    ...

There appears to currently be 4 different google adapters:

    # discouraged, for backward compatibility
    ADAPTERS['gae'] = GoogleDatastoreAdapter
    # add gae adapters
    ADAPTERS['google:datastore'] = GoogleDatastoreAdapter
    ADAPTERS['google:datastore+ndb'] = GoogleDatastoreAdapter
    ADAPTERS['google:sql'] = GoogleSQLAdapter

So the test: .db._dbname != 'gae', will fail for three of the four google adapters. gluon/validators.py appears to have already solved a similar problem once with this test:

613: from pydal.adapters import GoogleDatastoreAdapter
    ...

617: if GoogleDatastoreAdapter is not None and isinstance(self.dbset.db._adapter, GoogleDatastoreAdapter):
    ...

Though, that is still a somewhat clunky way to solve the problem. If applications are going to need to know that they are using a GAE adapter, then there should likely be better support for determining that fact from PYDAL. But should the applications need to know this? I don't know. What is the PYDAL philosophy for dealing with backend specific code in the application?

gi0baro commented 9 years ago

@stephenrauch thank you for the accurate analysis. I think dealing with specific db engine code is unavoidable, since we support very different engines. Also, I don't think this require any update on pydal, since I can't see any reason to simplify those checks from external code, as 99% of times under normal usage, you actually know which engine you're dealing with in your application code.

Finally, I think this should be fixed on web2py's code only. @mdipierro do you agree?

stephenrauch commented 9 years ago

The basic problem is that framework code like Web2py shouldn't really have any code specific to the backend used since it is outside of Web2py control. But in certain instances, Web2py will need to alter its behavior based on backend characteristics. One example is the function def can_join():. This is a backend capability query which allows the framework to change its behavior in a backend compatible way.

So while I agree the Web2py code should change, it may still be necessary for the pydal code to provide support for the change. I looked at the above mentioned code in gluon/validators.py, but it wasn't apparent to me what gae deficiency the specific code was working around, so I couldn't readily suggest a solution.

Maybe someone else could suggest one or more capability queries which could solve the problem in gluon/validators.py.

stephenrauch commented 9 years ago

The title on this issue looks to be a point of confusion. The link at the beginning of this post, links to a post by David Manns. The post has nothing to do with decimal, but does have the stack trace that was analyzed above.

David Manns has another post about the same time about decimal, and converting/migrating integers to same, that is here:

https://groups.google.com/forum/?fromgroups=#!topic/web2py/_YHapb2KGPM

I don't know if we can change the title of this issue, but if so, it should be considered.

davidmanns commented 8 years ago

Stephen Rauch is correct, this issue is mis-titled. Its still open and I just confirmed still broken in web2py 2.14.5. For me this means I'm stuck with web2py 2.9.12 in production as I can't think of an acceptable workaround for setting up the appropriate validators in my form:

def transearch(): chkurlreader()

revexpaccts = db(db.CoA.Type == 'Revenue/Expense')
banks = db(db.CoA.Type == 'Bank')
unalloc = db(db.CoA.Name == 'Unallocated').select().first()

form=SQLFORM.factory(
    Field('bank', 'reference CoA', requires=IS_EMPTY_OR(IS_IN_DB(banks, 'CoA.id','%(Name)s'))),
    Field('account', 'reference CoA', requires=IS_EMPTY_OR(IS_IN_DB(revexpaccts, 'CoA.id','%(Name)s')),
                        default = unalloc.id),
    Field('event', 'reference Events',
                        requires=IS_EMPTY_OR(IS_IN_DB(db, 'Events.id', '%(Event)s', orderby=~db.Events.Date))),
    Field('startdate', 'date',requires=[IS_NOT_EMPTY(),
                                        IS_DATE_IN_RANGE(format=T('%Y-%m-%d'),
                                            minimum=datetime.date(2011,11,1))],
                        default=datetime.date(request.now.year, 1, 1),
                        comment='date range ignored if searching unallocated or event'),
    Field('enddate', 'date',requires=[IS_NOT_EMPTY(),
                                      IS_DATE_IN_RANGE(format=T('%Y-%m-%d'),
                                            minimum=datetime.date(2012,1,1))],
                        default=request.now),
    Field('accrual', 'boolean', default=False,
                        comment='tick to select all accruals, all other parameters are ignored'))
davidmanns commented 8 years ago

error log.txt

Above is the error log from the current web2py version 2.14.5.

Yes, the 'gae' test is bad. But the 'else' clause is clearly an earlier attempt to make IS_IN_DB work in the GAE case. I forced the code to take the 'else' path by changing 'gae' to 'google:datastore' which is the value observed. It still failed. So the fix is going to involve more than just the test for the gae environment.

Note that IS_IN_DB(db, ...) works fine, it is only IS_IN_DB(set, ...) that breaks. So its possible that changing the 'set' implementation might avoid the need for GAE specific code.