revert: display information about IntegrityError

[blueyed]

It would be good to have the information about the IntegrityError in the error
message, instead of the generic "missing dependency":

> …/Vcs/django-reversion/reversion/models.py(32)

  24     def _safe_revert(versions):                                                                                                                                                          
  25         unreverted_versions = []                                                                                                                                                         
  26         __import__('pdb').set_trace()                                                                                                                                                    
  27         for version in versions:                                                                                                                                                         
  28             try:                                                                                                                                                                         
  29                 with transaction.atomic(using=version.db):                                                                                                                               
  30                     version.revert()                                                                                                                                                     
  31             except (IntegrityError, ObjectDoesNotExist):                                                                                                                                 
  32  ->             unreverted_versions.append(version)                                                                                                                                      
  33         if len(unreverted_versions) == len(versions):                                                                                                                                    
  34             raise RevertError(ugettext("Could not save %(object_repr)s version - missing dependency.") % {                                                                               
  35                 "object_repr": unreverted_versions[0],                                                                                                                                   
  36             })                                                                                                                                                                           
  37         if unreverted_versions:                                                                                                                                                          
  38             _safe_revert(unreverted_versions)                                                                                                                                            
 IntegrityError: duplicate key value violates unique constraint "app_subscriptionplan_organization_id_66f20bda_uniq"
DETAIL:  Key (organization_id, name)=(77, WEEKLY) already exists.

[etianen]

The trouble is, this isn't information that should really be shown to an end user. I guess the logs is the correct place for this.

…

On 4 September 2018 at 20:04, Daniel Hahler ***@***.***> wrote: It would be good to have the information about the IntegrityError in the error message, instead of the generic "missing dependency": > …/Vcs/django-reversion/reversion/models.py(32) 24 def _safe_revert(versions): 25 unreverted_versions = [] 26 __import__('pdb').set_trace() 27 for version in versions: 28 try: 29 with transaction.atomic(using=version.db): 30 version.revert() 31 except (IntegrityError, ObjectDoesNotExist): 32 -> unreverted_versions.append(version) 33 if len(unreverted_versions) == len(versions): 34 raise RevertError(ugettext("Could not save %(object_repr)s version - missing dependency.") % { 35 "object_repr": unreverted_versions[0], 36 }) 37 if unreverted_versions: 38 _safe_revert(unreverted_versions) IntegrityError: duplicate key value violates unique constraint "lock8_subscriptionplan_organization_id_66f20bda_uniq" DETAIL: Key (organization_id, name)=(77, WEEKLY) already exists. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#750>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJFCFAi8eJ-Y4awDldSTpauaMIdEqWXks5uXs7agaJpZM4WZkpl> .

[blueyed]

Isn't it (typically) a staff user, and therefore more OK?
I've thought like having this in some extra ("Details: %s") information.
Logging however would be better already of course.

[blueyed]

Maybe it could be logged in general (logger.exception), and attached/wrapped into the RevertError to be displayed in the admin only?

To be clear: I am only talking about the "if len(unreverted_versions) == len(versions)" case.

[etianen]

I'd take a pull request to add logger.exception() calls for the errors. I'm not so keen on displaying the postgres error message in the admin, as it could be very big and intimidating for admin users (who are typically not database-savvy).

…

On 5 September 2018 at 15:15, Daniel Hahler ***@***.***> wrote: Maybe it could be logged in general (logger.exception), and attached/wrapped into the RevertError to be displayed in the admin only? To be clear: I am only talking about the "if len(unreverted_versions) == len(versions)" case. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#750 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJFCJ7KmzgTCaA9hvDbXlzlj14U_fR2ks5uX9yVgaJpZM4WZkpl> .

[tony]

@blueyed @etianen

I've followed up with a PR to this in #921

This was some time ago, but I've bumped into this a couple of times over the years - and always had to omit fields and just save everything.

Adding a logging.exception() to the IntegrityError instantly diagnosed this in my case.