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

datastore: Prevent unicode/ascii conversion errors #2904

Merged
merged 2 commits into from
Mar 11, 2016

Conversation

mordae
Copy link
Contributor

@mordae mordae commented Mar 10, 2016

Some error messages were causing the errors due to format conversion of unicode fields into str templates.

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

@@ -123,7 +123,7 @@ def _cache_types(context):
if 'nested' not in _type_names:
native_json = _pg_version_is_at_least(connection, '9.2')

log.info("Create nested type. Native JSON: {0}".format(
log.info(u"Create nested type. Native JSON: {0}".format(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure the ValidationError changes below are correct, but is it safe to pass unicode to any logger that's configured? Have you tested this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using repr() for user-supplied values sent to logs. That seems safer.

@wardi wardi self-assigned this Mar 10, 2016
@mordae
Copy link
Contributor Author

mordae commented Mar 10, 2016

You are right, it's usually not a good idea. Do you prefer .format(repr(x)) or just %r?

@TkTech
Copy link
Member

TkTech commented Mar 10, 2016

@mordae {0!r} is the same as %r/repr(x)

Some error messages were causing the errors due to format conversion
of unicode fields into str templates.

Signed-off-by: Jan Dvořák <mordae@anilinux.org>
@mordae
Copy link
Contributor Author

mordae commented Mar 10, 2016

OK, let's make it simpler. These are just the ValidationError formats.

@mordae
Copy link
Contributor Author

mordae commented Mar 10, 2016

And this should take care of the log messages as per your instruction. Is this acceptable?

Logging statements had a potential to crash on unicode inputs.
Make sure they are always escaped.

Signed-off-by: Jan Dvořák <mordae@anilinux.org>
@@ -1310,8 +1313,7 @@ def make_private(context, data_dict):


def make_public(context, data_dict):
log.info('Making resource {0} public'.format(
data_dict['resource_id']))
log.info('Making resource {resource_id!r} public'.format(**data_dict))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you been using something other than UUIDs for your resource_ids?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, just being consistent and quoting all of them. It's UnicodeText in the model...

wardi added a commit that referenced this pull request Mar 11, 2016
datastore: Prevent unicode/ascii conversion errors
@wardi wardi merged commit 288b26e into ckan:master Mar 11, 2016
@wardi
Copy link
Contributor

wardi commented Mar 11, 2016

@amercader This puts python reprs in our logs now, do you think that's a concern for backporting?

@mordae
Copy link
Contributor Author

mordae commented Mar 11, 2016

As for the ValidationError fixes; yes, the unicode-related crashes are easily reproducible. As for the other commit I frankly do not know. Thanks for the merge, by the way. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants