Skip to content

prevent downstream nil exception when view_definition is nil. #332

New issue

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

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

Already on ? Sign in to your account

Merged
merged 1 commit into from
Aug 27, 2014

Conversation

coldnebo
Copy link
Contributor

Fixes #330. it looks like there is more than one way to collect column names, but I'm not sure what 'real' column names are in the context of schema_statements.rb#L253. Is my fix ok, or does it mask other issues?

@coldnebocoldnebo changed the title fix for #330 prevent downstream nil exception when view_definition is nil. May 19, 2014
annaswims added a commit that referenced this pull request Aug 27, 2014
prevent downstream nil exception when view_definition is nil.
@annaswimsannaswims merged commit bda0dac into rails-sqlserver:master Aug 27, 2014
@metaskills
Copy link
Member

Not sure if guarding against these are a good idea. See:

Not sure, but the general history I remember is that being silent on this core issue has caused more grieve than it saved.

@coldnebo
Copy link
Contributor Author

The warn behavior in the fix for #73 is unchanged by this fix. However, if you are saying that it's better to warn the user during find() rather than silencing the downstream error, I agree.

It might be better to duplicate the warning here too.

coldnebo added a commit to coldnebo/activerecord-sqlserver-adapter that referenced this pull request Aug 27, 2014
@coldnebo
Copy link
Contributor Author

What do you think of this approach?

@metaskills
Copy link
Member

It might be better to duplicate the warning here too.

Yea, that might be the best way. Raising an error in both places caused issues with those that could not set permissions due to legacy and/or overzealous DBA's. Likewise, being silent has caused errors and confusion too. So connecting up a second warn that pointed to the first might be best.

coldnebo added a commit to coldnebo/activerecord-sqlserver-adapter that referenced this pull request Aug 27, 2014
Sign up for free to join this conversation on . Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

view_information should raise instead of warn
3 participants