fix #95 don't refrence config object inappropriately

[ralphsmith80]

Also updated the docs.

[dkellner]

Really sorry to respond to this so late - I just recently got rights to merge PRs and try to clean things up at the moment.

What is the reasoning behind this change? And does it apply to other config items as well? I'm thinking if ID_FIELD in particular. At a first glance it appears we do the same thing for this one, too.

[ralphsmith80]

It's been a while since I've looked at this, but yes this is likely an issue with all config items being set in the way this one was. If I remember correctly the overarching issue is that eve-python allows complete configuration through settings file and if on override was provided in the settings file it was not being honored.

[dkellner]

Thank you for your quick reply to an old thread like this :). Do you have time to look into this again? If not I will take over and rebase your changes on current master. And add a test for this bug as well.

[ralphsmith80]

@dkellner, unfortunately I don't really have a lot of time to play around with this these days and the environment I had setup for this is not longer around so it's not a quick jump for me to get back into it. But if you have questions around the reasoning behind my pull requests I'd be happy to help.

[ralphsmith80]

Looks like I might have retry some of this stuff after all. I'm attempting to try this out again so I can try to sell this to a new team, but I think I'm running into the ITEM_LOOKUP_FIELD issue again. I'm using my branch so I didn't expect that and I'm still refreshing my memory on this stuff, but I'll keep you posted via github.

[ralphsmith80]

We'll as I expected this was my fault and was than expected just took some dusting off. I'm working with INTEGER id's and I was not using the right ITEM_LOOKUP_FIELD regex. Eve is a great project.

[dkellner]

Thank you for getting back to this! You mention you're using your own branch - are there other issues in the current master branch that force you to use another codebase?

[ralphsmith80]

I did an install of master (pip install git+http://github.com/RedTurtle/eve-sqlalchemy.git) because I was having some other issues with my current database models, but the issues were still present.

In a nut-shell most of my issues revolve are how ID_FIELD|id_field and ITEM_LOOKUP_FIELD|item_lookup_field are being referenced. At first glance it looks like that's partially being addressed in the updates made to master by the introduction of the _id_field method.

However, based on my quick check it's still not completely addressing the issue. Again just at a quick glance this requires that the id_field is set at the resource level. It should probably be something like: (fyi, I didn't validate this code)

def _id_field(self, resource):
  id_field = self.driver.app.config['ID_FIELD']
  if (hasattr(self.driver.app.config['DOMAIN'][resource], 'id_field')):
    id_field = self.driver.app.config['DOMAIN'][resource]['id_field']
  return id_field

This would use the resource id_field only if it was provided otherwise the global ID_FIELD would be used.

One of the pull requests I submitted helped address this with the ID_FIELD, but I'm sure it wasn't all inclusive for every config option since it was only for ID_FIELD. Notice in this commit I'm making the fall back ITEM_LOOKUP_FIELD so it still honors the global setting.

Between one of the two the pull requests I submitted, I beleive it was pull request #97, there was the added benefit of not needing to do anything special to set the ID_FEILD like what is described in the docs. Specifically the following part which I find a bit odd, is not longer required.

config.ID_FIELD = ID_FIELD
config.ITEM_LOOKUP_FIELD = ID_FIELD

Also just to note there's another _get_id method in utils.py. Looks like a bit of duplication there that's going to be confusing. Although I haven't grepped the code enough to see if there's a use case for both methods.

Edited: fix code example

[ralphsmith80]

I wonder if there might be a better medium for communicating all this rather than just on this pull request, but for now anyway here's some more updates.

I rebased on master and most of what I had working seems to still be working. However, I did have to add the _client_projection method back in. That appears to have been removed from master and there are call to it in the find and find_one methods so that's probably broken now. Here's what that looks like.

def _client_projection(self, req):
        """ Returns a properly parsed client projection if available.

        :param req: a :class:`ParsedRequest` instance.

        .. versionadded:: 0.4
        """
        client_projection = {}
        if req and req.projection:
            try:
                client_projection = json.loads(req.projection)
            except:
                abort(400, description=debug_error_message(
                    'Unable to parse `projection` clause'
                ))
        return client_projection

Now back to the 'most' of it is working part.

I'm getting embedded ids, but I'm not longer getting the expanded version when I issue an embedded={"groups":1} query. This is a requirement for my use case, and works in my branch. I have not yet dug into what change caused that yet. I can't find it now, but I did read somewhere that projections seemed to be related to embedding, but that doesn't fix the issue either. Maybe I'm missing something there.

[dkellner]

Just one quick note about _client_projection: I've removed it because it was moved to the DataLayer class with Eve 0.6.1: https://github.com/nicolaiarocci/eve/blob/v0.6.4/eve/io/base.py#L464

[ralphsmith80]

I've removed it because it was moved to the DataLayer class with Eve 0.6.1:

Understood, but I was getting an error after the rebase for invocations of self._client_projection in find and find_one. Might be that something was messed up while rebasing.

[ralphsmith80]

For completeness here are two basic examples of how I've attempted to use eve-sqlalchemy that relate to the changes I've had to make.

I've used Postgres and MySQL, but as you might expect the SQL database does not matter in either of these cases.

Example 1

This is a very basic example overriding ID_FIELD and ITEM_LOOKUP_FIELD. By default these values are set to _id. As I user I should only be concerned with providing the value at the global, resource, or resource item level.

In eve-sqlalchemy the general behavior for getting any setting value from the config should be check if it's provided at the resource/resource item level e.g. id_field; and if it's not present then use the global setting e.g. ID_FIELD.

These are the test queries that should work with this example

http://localhost:5000/a
http://localhost:5000/a/1

Implementation

# SQLAlchemy Imports
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy import (
  Column,
  )
from sqlalchemy.dialects.mysql import (
  INTEGER,
  )

# Eve imports
from eve import Eve
from eve_sqlalchemy import SQL
from eve_sqlalchemy.validation import ValidatorSQL

# Eve-SQLAlchemy imports
from eve_sqlalchemy.decorators import registerSchema

Base = declarative_base()

@registerSchema('a')
class A(Base):
  __tablename__ = 'a'
  id = Column(INTEGER(10), nullable=False, primary_key=True)

SETTINGS = {
  'XML': False,
  'HATEOAS': False,
  'ID_FIELD': 'id',
  'ITEM_LOOKUP_FIELD': 'id',
  'ITEM_URL': 'regex("[0-9]+")',
  'DOMAIN': {
    'a': A._eve_schema['a'],
  },
  # SQLALCHEMY_TRACK_MODIFICATIONS adds significant overhead set it to True to suppress warning.
  'SQLALCHEMY_TRACK_MODIFICATIONS': True,
  # User proper url for database
  'SQLALCHEMY_DATABASE_URI': 'mysql+pymysql://...',
}

app = Eve(auth=None, settings=SETTINGS, validator=ValidatorSQL, data=SQL)

db = app.data.driver
Base.metadata.bind = db.engine
db.Model = Base
db.create_all()

if __name__ == '__main__':
  app.run(host='0.0.0.0', port=5000)

Example 2

This is an example that illustrates a database model where every tables primary key is unique. i.e. table A has a primary key a_id and table B has a primary key b_id. AtoB is a reference table that maps A rows to B rows (many to many relationship). Additionally A has a foreign key relationship on itself (one to many) via pa_id.

The embedded parameter was problematic in this case and required additions to ensure the correct id_field was being used when GETing the embedded resource.

These are the test queries that should work with this example

http://localhost:5000/a
http://localhost:5000/a/1
http://localhost:5000/a?where={"pa_id":0}
# GET 'a' where a_id = 1 and embed all 'a' where pa_id == 1
http://localhost:5000/a/1?embedded={"a":1}
# GET 'a' where a_id = 1 and embed all 'b' where a_id == b_id from AtoB reference table (many to many)
http://localhost:5000/a/2?embedded={"b":1}
http://localhost:5000/b
http://localhost:5000/b/21

Implementation

# SQLAlchemy Imports
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import relationship
from sqlalchemy import (
  Table,
  Column,
  ForeignKey,
  )
from sqlalchemy.dialects.mysql import (
  INTEGER,
  )

# Eve imports
from eve import Eve
from eve_sqlalchemy import SQL
from eve_sqlalchemy.validation import ValidatorSQL

# Eve-SQLAlchemy imports
from eve_sqlalchemy.decorators import registerSchema

Base = declarative_base()

AtoB = Table('t_a_b', Base.metadata,
    Column('a_id', INTEGER(10), ForeignKey('t_a.a_id'), primary_key=True, nullable=False),
    Column('b_id', INTEGER(10), ForeignKey('t_b.b_id'), primary_key=True, nullable=False))

@registerSchema('t_b')
class B(Base):
  __tablename__ = 't_b'
  b_id = Column(INTEGER(10), nullable=False, primary_key=True)

@registerSchema('t_a')
class A(Base):
  __tablename__ = 't_a'
  a_id = Column(INTEGER(10), nullable=False, primary_key=True)
  pa_id = Column(INTEGER(10), ForeignKey('t_a.a_id'), nullable=True)

  a = relationship('A', primaryjoin='A.a_id==A.pa_id')
  b = relationship(B, secondary=AtoB)

SETTINGS = {
  'XML': False,
  'HATEOAS': False,
  'ITEM_URL': 'regex("[0-9]+")',
  'DOMAIN': {
    't_a': A._eve_schema['t_a'],
    't_b': B._eve_schema['t_b'],
  },
  # SQLALCHEMY_TRACK_MODIFICATIONS adds significant overhead set it to True to suppress warning.
  'SQLALCHEMY_TRACK_MODIFICATIONS': True,
  # User proper url for database
  'SQLALCHEMY_DATABASE_URI': 'mysql+pymysql://...',
}
SETTINGS['DOMAIN']['t_b'].update({
  'url': 'b',
  'id_field': 'b_id',
  'item_lookup_field': 'b_id',
})
SETTINGS['DOMAIN']['t_a'].update({
  'url': 'a',
  'id_field': 'a_id',
  'item_lookup_field': 'a_id',
})

app = Eve(auth=None, settings=SETTINGS, validator=ValidatorSQL, data=SQL)

db = app.data.driver
Base.metadata.bind = db.engine
db.Model = Base
db.create_all()

if __name__ == '__main__':
  app.run(host='0.0.0.0', port=5000)

[dkellner]

I finally looked into your two examples. The first can be solved by applying the changes in your two PRs, but how about the second one? For me, it did not work even with this changes (which is essentially your two PRs combined):

diff --git a/eve_sqlalchemy/__init__.py b/eve_sqlalchemy/__init__.py
index d8b8b2d..24d79d2 100644
--- a/eve_sqlalchemy/__init__.py
+++ b/eve_sqlalchemy/__init__.py
@@ -94,10 +94,13 @@ class SQL(DataLayer):
             if 'datasource' in v and 'source' in v['datasource']:
                 source = v['datasource']['source']
                 source = app.config['DOMAIN'].get(source.lower(), {})
-                for key in ('schema', 'id_field', 'item_lookup_field',
-                            'item_url'):
-                    if key in source:
-                        v[key] = source[key]
+                v['schema'] = source.get('schema', {})
+                v['id_field'] = source.get(
+                    'id_field', app.config.get('ID_FIELD'))
+                v['item_lookup_field'] = source.get(
+                    'item_lookup_field', app.config.get('ITEM_LOOKUP_FIELD'))
+                v['item_url'] = source.get(
+                    'item_url', app.config.get('ITEM_URL'))
             # Even if the projection was set by the user, require that:
             # - the id field is included
             # - the ETag is excluded, as it will be added automatically if
diff --git a/eve_sqlalchemy/decorators.py b/eve_sqlalchemy/decorators.py
index 38c279e..1ea19e3 100644
--- a/eve_sqlalchemy/decorators.py
+++ b/eve_sqlalchemy/decorators.py
@@ -56,10 +56,6 @@ class registerSchema(object):
             resource: {
                 'schema': {},
                 'datasource': {'source': cls_.__name__},
-                'id_field': config.ID_FIELD,
-                'item_lookup': True,
-                'item_lookup_field': config.ID_FIELD,
-                'item_url': 'regex("[0-9]+")'
             }
         }
         projection = domain[resource]['datasource']['projection'] = {}
diff --git a/eve_sqlalchemy/tests/test_settings.py b/eve_sqlalchemy/tests/test_settings.py
index 7410fbd..82084d7 100644
--- a/eve_sqlalchemy/tests/test_settings.py
+++ b/eve_sqlalchemy/tests/test_settings.py
@@ -13,8 +13,8 @@ SQLALCHEMY_DATABASE_URI = 'sqlite:///'  # %s' % db_filename
 SERVER_NAME = 'localhost:5000'

 ID_FIELD = '_id'
-ITEM_LOOKUP = True
 ITEM_LOOKUP_FIELD = ID_FIELD
+ITEM_URL = 'regex("[0-9]+")'

 RESOURCE_METHODS = ['GET', 'POST', 'DELETE']
 ITEM_METHODS = ['GET', 'PATCH', 'DELETE', 'PUT']

Do you have a solution for this already or know what the problem is?

[ralphsmith80]

I made a change to my branch to support the second example, but I didn't submit a pull request.

The main change was to __init__.py to apply what I mentioned earlier; reference lower case DOMAIN property if it exists otherwise use the upper case global property.

def _id_field(self, resource):
  config = self.driver.app.config
  schema = config['DOMAIN'][resource]
  return schema.get('id_field', config['ID_FIELD'])