Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #825 (closed defect: fixed)

Opened 12 years ago

Last modified 11 years ago

[PATCH] fix saprovider + finalize abstraction

Reported by: jvanasco@… Owned by: godoy
Priority: normal Milestone: 1.0b1
Component: SQLObject Version: 0.9a5
Severity: normal Keywords:
Cc:

Description

This patch consolidates the issues from tickets 801 , 805, 806 , and also finalizes the identity model.py migration for sqlalchemy. someone else will have to do sqlobject - i don't know it well enough.

overview of exterior changes

model.py - User/user is now Useraccount/useraccount

app.cfg now supports these values

# settings to map identity onto model.py fields
identity.db.user_id="user_id" 
identity.db.user_name_field="user_name" 
identity.db.password_field="password" 

# settings to visit to model.py VisitIdentity object
identity.soprovider.model.visit2identity="${package}.model.VisitIdentity"
identity.saprovider.model.visit2identity ="${package}.model.VisitIdentity"

Attachments

identity_full.diff Download (20.1 KB) - added by jvanasco@… 12 years ago.
consolidated patches

Change History

Changed 12 years ago by jvanasco@…

consolidated patches

comment:1 Changed 12 years ago by jvanasco@…

whoops

the patch is fine, but the description above is wrong

identity.db.user_id="user_id" 
identity.db.user_name="user_name" 
identity.db.password="password

are the correct settings

comment:2 Changed 12 years ago by jvanasco@…

i'm really sorry about this. i missed this line -- my testing framework was calling in the wrong files.

saprovider.py

- global user_class, group_class, permission_class 
+ global user_class, group_class, permission_class , visit2identity_class

comment:3 Changed 12 years ago by godoy

At least for me, I'd like to see those functionalities broken into pieces... It is better to analyze and apply some parts of this patch. I also prefer small changes instead of big ones.

Anyway, this is too big to me, so I'll wait for other people with commit access to take a look at this and decide what to do... If patches were smaller then I could apply and test it patch by patch.

comment:4 Changed 12 years ago by kevin

I have to agree with Jorge... consolidating patches makes it a bit harder to apply.

comment:5 Changed 12 years ago by jvanasco@…

instead of giving patches, i'll just comment what i did, as some of this may have been taken care of by recent patches:

soprovider / saprovider

  • rename 'visit_class' to visit2identity_class as it is a lookup table for visit2identity

soprovider / saprovider / app.cfg / model.py

  • i renamed visit_identity to visit2identity. i also changed the namespace to be part of so/sa provider instead of visit. this is because there is no standard naming scheme in tg for database tables. association tables tend to either be named CamelCase classa_classb or classa2classb. We can't do camelcase in the db as there are different backends, and either several of the legacy tables have _ in them already or there are the 'tg_' prepended tables that kind of suggest joins, so i opted for the explict description

soprovider on a successful validation, there was no logging of the link. saprovider had it in this spot, so i added it:

+            log.info( "associating user (%s) with visit (%s)", user.user_name, 
+                  visit_key )

saprovider

  • the imports on the top of the file were way out of sync. i made them more in line with soprovider
  • i also added "from turbogears.util import load_class" as in soprovider, removed the _load_class function within the class, and replaced calls to _load_class with load_class
  • the SqlAlchemyIdentity? class has this: ( note, its in there 2x )
    visit= TG_VisitIdentity.get_by( visit_key= self.visit_key ) 
    
    that should really be importing the dynamic class as in soprovider
    visit= visit2identity_class.get_by( visit_key= self.visit_key ) 
    
  • create_provider_model
    • this depends on activemapper. activemapper doesn't work right now. i just made the class return early - otherwise the app will break
    • this also calls "TG_VisitIdentity.table.create()" directly, it should be calling the global class
      -            TG_VisitIdentity.table.create()
      +            visit2identity_class.table.create()
      
  • the linking of the user to the visit on successful validation also has this issue
  • this class also has a ton of TG_ tables defined in it. i deleted them all , as they're alread in model.py

savisit.py this is pretty straightforward -- i took out the activemapper functionality, and replaced it with the monkeypatch

-class TG_Visit(ActiveMapper):
-    class mapping:
-        visit_key= column( String(40), primary_key=True )
-        created= column( DateTime, default=datetime.now )
-        expiry= column( DateTime )
+tbl__tgvisit = Table('tg_visit', __engine__, 
+    Column('visit_key', String(40) , primary_key=True),
+    Column('created', DateTime  , nullable = False ),
+    Column('expiry', DateTime ),
+)
 
+class TG_Visit(object):
+    table = tbl__tgvisit
     def lookup_visit( cls, visit_key ):
         return TG_Visit.get( visit_key );
     lookup_visit= classmethod(lookup_visit)
+
+assign_mapper( TG_Visit , tbl__tgvisit )

model.py i fixed the imports for the database store that is chosen - it gives you the right stuff for sqlobject, but its broke for sqlalchemy

+
+#if $identity != "sqlalchemy"
 from sqlobject import *
 from turbogears.database import PackageHub
+#end if
 #if $identity == "sqlobject"
 from turbogears import identity
 #end if
 
+#if $identity != "sqlalchemy"
 hub = PackageHub("${package}")
 __connection__ = hub
 
 # class YourDataClass(SQLObject):
 #     pass
+#end if
+#if $identity == "sqlalchemy"
+from sqlalchemy import *
+from sqlalchemy.ext.activemapper import *
 
+from turbogears.database import PackageEngine 
+engine = __engine__ = PackageEngine("${package}")
+#end if

app.cfg

  • i removed visit.soprovider.model , and renamed it identity.soprovider.model.visit2identity -- as its not part of visit, its part of identity. The namespace was misleading.
  • "# identity.provider='${identity}'" is a commented line, showing the default provider -- i set it to not be a comment line if sqlalchemy is chosen, as tg will use sqlobject if sqlalchemy is commented out
  • i set 'identity.on=False' for /static and /favicon by default, as that just kills the server with superfluous db calls

The big this was this -- abstracting the login form:

i'm not showing all of the code here, as you will def. have other ideas on this, I'll just show how i did it:

app.cfg

+# The names of the fields on your user table that will be used for identity purpose
+# user_id must be a primary key, user_name and password can be named anything
+# The default values are those used by the base TG class installs (>= 0.9a5)
+# identity.db.user_id="user_id"
+# identity.db.user_name="user_name"
+# identity.db.password="password"

then in saprovider, i just renamed a few accessors

  • i set some variables and used them for lookups
    -        user= user_class.get_by( user_name=user_name )
    +        _dbfield_user_id = turbogears.config.get('identity.db.user_id','user_id') 
    +        _dbfield_user_name = turbogears.config.get('identity.db.user_name','user_name') 
    +        _dbfield_password = turbogears.config.get('identity.db.password','password') 
    +
    +        user= user_class.get_by( **{ _dbfield_user_name : user_name } )
    
    -        if user.password!=self.encrypt_password(password):
    +        if getattr(user,_dbfield_password) != self.encrypt_password(password):
    
    
    +        _dbfield_user_name = turbogears.config.get('identity.db.user_name','user_name') 
    +        return getattr(self.user,_dbfield_user_name)
    
    
  • in _get_user_name i replaced
    -            link.user_id= user.user_id
    +            link.user_id= getattr(user,_dbfield_user_id)
    

everything i did used the tg 0.9a5 stock values as defaults - so people who upgrade and don't specify a new option won't notice a thing

comment:6 Changed 12 years ago by godoy

Too many things for me again, so I'll just comment a few of them.

  • Class naming convention -> We are pursuing PEP8 recommendations, so there is the rule that should apply to our code in TG. Independente code (widgets, plugins, etc.) aren't tied to this commitment, but code written for TG should be changing and being written following these rules.
  • Using "_" gives the idea of a JOIN --> Not to me. Neither in the naming scheme I and other developers and DBAs here use, so this is really subjective.
  • Changing class names --> I don't see why it needs to be done, specially with things that aren't exposed (visit and identity are tied internally, you don't have to have visit to have identity -- just if you use it as one of the means to find if a user is authenticated or not).
  • Removing classes because they are available in the model --> What about backwards compatibility? People who started their projects earlier should do what? You've removed something that they didn't have to care before, but now they need to look in the old version's code, find where what they need is defined, copy it to their model.py and then they'll be able to have their functionality back.
  • Making the same functionality available for SO and SA --> I'm +1 for patches that do that and only that, i.e., no renaming, no relocation. This is a first step. Renaming, relocation, etc. is a second step and should be done thinking about existent applications.
  • Factoring code and reusing already existing code --> I'm +1 for patches for that as well. Specially if all tests passes and there's no drawback on that approach (sometimes code reuse bring collateral effects and code duplication is intentional).
  • Fixing table creation --> I'm +1 for a patch for that and only that, without any renaming. Renamings should be done later, in a separate batch of changes.
  • Using "" as in "tbltgvisit" --> I don't like and don't see a reason for it. What is the meaning of "" here? Python has special meanings for "" prefixing a name, making it private, "_" making it non-public. Those are conventions widely used in Python code and I believe it is a good thing to stick to them, since this is widely documented. But it is just my opinion, I don't speak officially for TG or for the other developers.
  • identity.on = False for static --> I'm against that, at least another person on the mailing list was as well. This prevent people from direct linking to your static resources. A patch for the docs talking about this possibility is welcome, but I don't believe that it should be changed in the configuration files.
  • Too many things to read and comment on a web interface is a really bad and annoying thing. Discussions like that would be much more productive at the trunk mailing list with an hyperlink here pointing to the discussion. Decisions should be placed back here either as patches, commit notes or even some text, just for feedback.
  • These are mine and mine only opinions. Other people might agree or not with this...

comment:7 Changed 12 years ago by jvanasco@…

a- _ is currently being used both in the PEP style word_seperation and as a semantic identifier for a join in visit_identity. i don't care which one is more important to tg , but you can't use two conflicting naming conventions. i'm just trying to clean it up -- some parts of code suggest use in one context, others in another. if i look at a new db and i see a visit table, a identity table, and a visit_identity table, i'm going to assume that _ is being used to mark joins. if i open up tg and see pep_style_variable_naming and then see visit_identity, i'm going to assume that visit_identity is not an associative table. using both contexts is confusing.

b- Removing classes because they are available in the model --> What about backwards compatibility? that was already decided in .9a5 to be irrelevant. if you look at the SOprovider in trunk, you'll see that all the classes were already stripped out and forced into model.py necessitating an upgrade. i migrated that change to SA. also to note though, SO provider still has tg_permissions in there, which is also in model.py -- i think someone forgot to delete that.

c- i used "tbl _ _" as a prefix to show that the variable just held a table definition. as the table was already prefixed with "tg _", i thought it would be sloppy and misleading to prefix it "tbl_tg_". since the class was named TG_Visit , i thought it would be even more confusing to name it tg_visit , so "tbl", which semantically says "hey, its a table" to the next person who looks at that code was an obvious choice.

d- identity.on = False for static -->This prevent people from direct linking to your static resources. no, it causes a database hit on every page for every resource. have some gifs + css on your page? there's a db query for each one. i averaged 24 database calls per page for identity alone. Thats completely unnecessary. if you have static resources that you want to lock down, its 2 lines in app.config to lock it down to put it back in on a per-directory basis ( some sort of image directory or otherwise ) .

e- i didn't rename any classes. i just renamed some variables used to lookup classes

comment:8 Changed 12 years ago by godoy

  • Owner changed from anonymous to godoy

comment:9 Changed 12 years ago by godoy

I got to one point where I'm having a problem creating tables. Something related to the way that ActiveMapper? maps many_to_many relations, I believe.

A lot of this was commited in [1491] and [1492] for 1.0 and the trunk, respectively.

comment:10 Changed 12 years ago by godoy

  • Component changed from TurboGears to SQLObject

Now that I've already commited it, I see that I could have left the ActiveMapper? stuff... I won't reverse that part of the code until we see that the loop problem is not related to ActiveMapper?'s code or its usage in TG.

BTW, I've changed the component to SQL Object since this is more related to an ORM level than Turbogears...

comment:11 Changed 11 years ago by kevin

Should this ticket be closed? There are other tickets about the SQLAlchemy 0.2 migration that are more recent, and I'm not sure if there's something left here to commit?

comment:12 Changed 11 years ago by kevin

  • Status changed from new to closed
  • Resolution set to fixed

I think everything that's going to be taken from this particular patch has been.

Note: See TracTickets for help on using tickets.