Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ColanderAlchemy throws when encountering synonyms #35

Closed
ztane opened this issue Nov 5, 2013 · 10 comments
Closed

ColanderAlchemy throws when encountering synonyms #35

ztane opened this issue Nov 5, 2013 · 10 comments

Comments

@ztane
Copy link

ztane commented Nov 5, 2013

name = synonym('display_name',
       descriptor=property(_get_name, _set_name))

=>

File ".../colanderalchemy/schema.py", line 133, in add_nodes
getattr(self.inspector.relationships, name)
File ".../SQLAlchemy-0.8.1_post1-py2.7-linux-x86_64.egg/sqlalchemy/util/_collections.py", line 172, in __getattr__
  raise AttributeError(key)
AttributeError: name

Prolly should just continue on AttributeError there.

@tisdall
Copy link
Collaborator

tisdall commented Nov 6, 2013

@ztane Can you post your entire table code so I can test against it?

@tisdall
Copy link
Collaborator

tisdall commented Nov 6, 2013

@stefanofontanelli Would it make sense to map synonym attributes into the Colander schema as well as the base column properties? There's instances where people make the synonym because they want to do some sort of processing as part of the getter or setter and so it's conceivable that they'd want the Deform field to map to it instead of the base column (which would bypass that functionality). On the flip-side, though, some people use the synonym to create an attribute that gives them some interpretation on the other columns and isn't supposed to be used to set data (example would by having an x,y value for a vector and a synonym that returns the length of the vector).

@stefanofontanelli
Copy link
Owner

@tisdall I think we should add support for synonyms. Users can disable/exclude such fields if they don't want to map in Colander.

@tisdall
Copy link
Collaborator

tisdall commented Nov 7, 2013

@stefanofontanelli Should we specify the type of the synonym to be the same as the column it's synonym'izing and then expect people to override that if it's not correct?

@stefanofontanelli
Copy link
Owner

Yes, it seems a good solution.

On Thu, Nov 7, 2013 at 5:22 PM, tisdall notifications@github.com wrote:

@stefanofontanelli https://github.com/stefanofontanelli Should we
specify the type of the synonym to be the same as the column it's
synonym'izing and then expect people to override that if it's not correct?


Reply to this email directly or view it on GitHubhttps://github.com//issues/35#issuecomment-27980301
.

Stefano Fontanelli
Backend Developer | Gild, Inc.
800-664-2366 | cell *+1-415-547-0722 (US), +39-333-365-3294* (IT)

465 California Street, Suite 1250
San Francisco, CA 94104
www.gild.com

@tisdall
Copy link
Collaborator

tisdall commented Nov 7, 2013

@stefanofontanelli
There appears to be no way to pass in an info argument when using synonym(). The result of synonym() has an info property, but you have to manually change that after you've created the SynonymProperty object. I don't even know how'd you do it with the decorator version.

The other issue is the functionality of synonym's can also be done with hybrid properties and those have no concept of an info attribute.

Basically, both would be messy if the developer uses the setup_schema method.

There's also the whole mess of objectify and how it would try writing data to the same column twice (once directly, and once via the synonym) and it being rather non-deterministic which would be done first.

Maybe a better default would be to ignore synonym's and hybrid's, but allow for people to include them somehow via the override argument to SQLAlchemySchemaNode?

@ztane
Copy link
Author

ztane commented Nov 7, 2013

I reported the bug because I wanted to evaluate ColanderAlchemy with our existing models and CA crashed on the synonym. However, I am not yet sure when and if I want synonyms/hybrid properties in colander schema.

@tisdall
Copy link
Collaborator

tisdall commented Nov 7, 2013

I think for right now, it'd be prudent to just implement a second try-except with a continue as suggested in the original report. This is actually what the dictify() method is currently doing.

@stefanofontanelli
Copy link
Owner

@tisdall Yes, it is probably true.

On Thu, Nov 7, 2013 at 9:33 PM, tisdall notifications@github.com wrote:

I think for right now, it'd be prudent to just implement a second
try-except with a continue as suggested in the original report. This is
actually what the dictify() method is currently doing.


Reply to this email directly or view it on GitHubhttps://github.com//issues/35#issuecomment-28003616
.

Stefano Fontanelli
Backend Developer | Gild, Inc.
800-664-2366 | cell *+1-415-547-0722 (US), +39-333-365-3294* (IT)

465 California Street, Suite 1250
San Francisco, CA 94104
www.gild.com

tisdall added a commit to tisdall/ColanderAlchemy that referenced this issue Jan 21, 2014
…schema

- use Colander defaults wherever explicit settings are not given
- added tests for confirming documentation examples
- added fix and test for issue stefanofontanelli#35 (thrown exception on encountering synonym() )
- made changes to accommodate SQLAlchemy >= 0.9a
@tisdall
Copy link
Collaborator

tisdall commented Jan 22, 2014

This should be fixed in the master now, so I'm going to close the issue. Please test it out and let me know if there are further issues.

@tisdall tisdall closed this as completed Jan 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants