copr / copr

Clone

#2024 frontend, python: print human-readable validation errors in APIv3
Merged 2 years ago by praiskup. Opened 2 years ago by frostyx.
copr/ frostyx/copr human-readable-errors  into  main

file modified
+7 
@@ -56,6 +56,13 @@ 
 

          rlRun "copr-cli create --chroot $CHROOT --repo 'copr://${NAME_PREFIX}Project1' ${NAME_PREFIX}Project3"
 

          ### left after this section: Project1, Project2, Project3
 

  

+         # Test validation output
 

+         OUTPUT=`mktemp`
 

+         rlRun "copr-cli create ${NAME_PREFIX}Project1 --chroot wrong-chroot-name 2> $OUTPUT" 1
 

+         rlRun "cat $OUTPUT |grep -E '^Error: $'"
 

+         rlRun "cat $OUTPUT |grep \"\- name: Group copr (fas: gitcopr) already have project named\""
 

+         rlRun "cat $OUTPUT |grep \"\- chroots: 'wrong-chroot-name' is not a valid choice for this field\""
 

+ 

          rlRun "yes | copr-cli new-webhook-secret ${NAME_PREFIX}Project1 | grep -E 'Generated new token: .*-.*-.*-.*-.*'"
 

  

          ### ---- BUILDING --------------- ###

file modified
+21 
@@ -45,6 +45,27 @@ 
 

      _code = 500
 

  

  

+ class InvalidForm(BadRequest):
 

+     """
 

+     An exception raised by APIv3 code when form validation fails
 

+     """
 

+ 

+     def __init__(self, form):
 

+         super().__init__()
 

+         self.form = form
 

+         self.message = self._message()
 

+ 

+     def _message(self):
 

+         """
 

+         Create a human-readable error message from validators
 

+         """
 

+         result = []
 

+         for key, value in self.form.errors.items():
 

+             for message in value:
 

+                 result.append("{0}: {1}".format(key, message))
 

+         return "\n".join(result)
 

+ 

+ 

  class InsufficientStorage(CoprHttpException):
 

      """When there is not enough space left on the server for the src rpm."""
 

      _default = "Not enough space left"

file modified
+2 -2 
@@ -5,7 +5,7 @@ 
 

  from coprs import forms, db_session_scope
 

  from coprs.views.apiv3_ns import apiv3_ns, get_copr, file_upload, POST
 

  from coprs.views.misc import api_login_required
 

- from coprs.exceptions import DuplicateException, BadRequest
 

+ from coprs.exceptions import DuplicateException, BadRequest, InvalidForm
 

  from coprs.logic.modules_logic import ModuleProvider, ModuleBuildFacade
 

  

  
@@ -22,7 +22,7 @@ 
 

      copr = get_copr(ownername, projectname)
 

      form = forms.get_module_build_form(meta={'csrf': False})
 

      if not form.validate_on_submit():
 

-         raise BadRequest(form.errors)
 

+         raise InvalidForm(form)
 

  

      facade = None
 

      try:

file modified
+2 -1 
@@ -8,6 +8,7 @@ 
 

          DuplicateException,
 

          ApiError,
 

          UnknownSourceTypeException,
 

+         InvalidForm,
 

  )
 

  from coprs.views.misc import api_login_required
 

  from coprs import db, models, forms, helpers
 
@@ -176,7 +177,7 @@ 
 

              raise BadRequest(str(e))
 

          db.session.commit()
 

      else:
 

-         raise BadRequest(form.errors)
 

+         raise InvalidForm(form)
 

      return flask.jsonify(build_to_dict(build))

file modified
+4 -4 
@@ -1,12 +1,12 @@ 
 

  import flask
 

- from . import query_params, get_copr, file_upload, GET, PUT
 

- from .json2form import get_form_compatible_data
 

  from coprs.views.misc import api_login_required
 

  from coprs.views.apiv3_ns import apiv3_ns
 

  from coprs.logic.complex_logic import ComplexLogic, BuildConfigLogic
 

- from coprs.exceptions import ObjectNotFound, BadRequest
 

+ from coprs.exceptions import ObjectNotFound, InvalidForm
 

  from coprs import db, forms
 

  from coprs.logic.coprs_logic import CoprChrootsLogic
 

+ from . import query_params, get_copr, file_upload, GET, PUT
 

+ from .json2form import get_form_compatible_data
 

  

  

  def to_dict(project_chroot):
 
@@ -88,7 +88,7 @@ 
 

      chroot = ComplexLogic.get_copr_chroot_safe(copr, chrootname)
 

  

      if not form.validate_on_submit():
 

-         raise BadRequest(form.errors)
 

+         raise InvalidForm(form)
 

  

      buildroot_pkgs = repos = comps_xml = comps_name = with_opts = without_opts = None
 

      if "buildroot_pkgs" in data:

file modified
+6 -5 
@@ -11,7 +11,8 @@ 
 

  from coprs.logic.users_logic import UsersLogic
 

  from coprs.exceptions import (DuplicateException, NonAdminCannotCreatePersistentProject,
 

                                NonAdminCannotDisableAutoPrunning, ActionInProgressException,
 

-                               InsufficientRightsException, BadRequest, ObjectNotFound)
 

+                               InsufficientRightsException, BadRequest, ObjectNotFound,
 

+                               InvalidForm)
 

  from . import editable_copr
 

  

  
@@ -130,7 +131,7 @@ 
 

      form = form_class(data, meta={'csrf': False})
 

  

      if not form.validate_on_submit():
 

-         raise BadRequest(form.errors)
 

+         raise InvalidForm(form)
 

      validate_chroots(get_input_dict(), MockChrootsLogic.get_multiple())
 

  

      bootstrap = None
 
@@ -185,7 +186,7 @@ 
 

      form = forms.CoprForm(data, meta={'csrf': False})
 

  

      if not form.validate_on_submit():
 

-         raise BadRequest(form.errors)
 

+         raise InvalidForm(form)
 

      validate_chroots(get_input_dict(), MockChrootsLogic.get_multiple())
 

  

      for field in form:
 
@@ -243,7 +244,7 @@ 
 

              db.session.rollback()
 

              raise err
 

      else:
 

-         raise BadRequest(form.errors)
 

+         raise InvalidForm(form)
 

  

      return flask.jsonify(to_dict(fcopr))
 

  
@@ -265,7 +266,7 @@ 
 

          else:
 

              db.session.commit()
 

      else:
 

-         raise BadRequest(form.errors)
 

+         raise InvalidForm(form)
 

      return flask.jsonify(copr_dict)
 

  

  @apiv3_ns.route("/project/regenerate-repos/<ownername>/<projectname>", methods=PUT)

file modified
+13 -1 
@@ -14,7 +14,19 @@ 
 

      """
 

      Raised when the API request doesn't proceed successfully
 

      """
 

-     pass
 

+     def __str__(self):
 

+         errors = self.result.error.split("\n")
 

+ 

+         # A list of errors signalizes a form validation error
 

+         # If there is only one error in the list, just return its value
 

+         if len(errors) == 1:
 

+             return str(errors[0])
 

+ 

+         # Show one error per line
 

+         result = ""
 

+         for error in errors:
 

+             result += "\n- {0}".format(error)
 

+         return result
 

  

  

  class CoprNoResultException(CoprException):

Fix #1922

Metadata Update from @frostyx:
- Pull-request tagged with: needs-tests
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

I don't know if I like this implementation. It seems too complicated.
Do you have a better idea how to do this?

Also, the output isn't polished yet

Before:

Something went wrong:
Error: {'name': ["You already have project named 'tracer'."], 'chroots': ["'[]' is not a valid choice for this field"]}

Now:

Something went wrong:
Error: 
- name: You already have project named 'tracer'.
- chroots: '[]' is not a valid choice for this field

Before:

Something went wrong:
Error: {'name': ["You already have project named 'tracer'."]}

Now:

Something went wrong:
Error: name: You already have project named 'tracer'.

I think the multi-line error looks fine but the "Error: name: something" in the single-line error doesn't look good.

Metadata Update from @frostyx:
- Pull-request tagged with: needs-work
2 years ago

rebased onto 5f3fe39b67c0c8789d51da8a36db7e485755da29
2 years ago

Build succeeded.

Also, this basically means, that I made a change that is not backward compatible. To avoid this, we could instead of returning list, return string as previously, but format the messages with "\n" between them.

Return an error message for a given exception., this is now returning both list and str ... (this list vs. str inconsistency always complicates return value handling)

Dunno. Not a strong opinion, but the "array of errors" doesn't bring too much value to anyone, compared to "newline separated list of errors"?

rebased onto 4fab228e782099a1ee447b58a327488b17c1190f
2 years ago

Build succeeded.

Dunno. Not a strong opinion, but the "array of errors" doesn't bring too much value to anyone, compared to "newline separated list of errors"?

I agree with you, I joined the list into a newline-separated string. The CLI output looks the same as I showed in the previous comment, but we don't introduce any incompatibilities in the API.

Metadata Update from @frostyx:
- Pull-request untagged with: needs-work
2 years ago

Metadata Update from @praiskup:
- Request assigned
2 years ago

rebased onto 09cb12d58844f7b74943059c62757b8c4c56cf69
2 years ago

Metadata Update from @frostyx:
- Pull-request untagged with: needs-tests
2 years ago

Test added, PTAL.

Build succeeded.

I don't like one-letter variables.

I would say that the result is just one error message, not messages.

When I try the new API code, it throws:
CoprRequestException: name: You already have project named 'Test'.
which is weird, I would expect just CoprRequestException: You already have project named 'Test'.
Do we really need that string before the actual error message? (I mean name:)
I'd prefer consistency, when I try to create a new project that already exists it throws:

Something went wrong:
Error: 
- name: You already have project named 'Test'.
- chroots: '[]' is not a valid choice for this field

and with chroot:

Error: name: You already have project named 'Test'.

I know that this behavior is expected but I find it a little bit confusing. Also, there is a dot at the end of the sentence with the name but with chroots, there is no dot.
Edited 2 years ago by schlupov

rebased onto fd5f6559a29fec0694bd8837ecc4dcc629bc40ea
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

I don't like one-letter variables.

True, me neither :D
Fixed

I would say that the result is just one error message, not messages.

Good catch, fixed.

Also, there is a dot at the end of the sentence with the name but
with chroots, there is no dot.

A bit unrelated issue but I will be happy to fix it. The question is,
should everything end with a dot or without it?

Do we really need that string before the actual error message? (I
mean name:)

The reason why I introduced the name: thing because some of our
validators don't have custom messages, e.g. try the following:

copr create foo --chroot fedora-rawhide-x86_64 --delete-after-days bar

With the PR as is, the output looks like this

Error: 
- delete_after_days: Not a valid integer value
- delete_after_days: Number must be between 0 and 60.

without the name of the field in the beginning, the error would be just

Error: 
- Not a valid integer value
- Number must be between 0 and 60.

which is IMHO confusing because if I mess up multiple fields at once,
I will have no idea to which of them, the messages are related.

I see multiple options how to deal with this

  1. The thing I implemented
  2. Showing the field: prefix only for validators that don't have a
    custom error message
  3. Making sure all of our validators define a custom error message
    that is understandable on its own

I don't have a strong preference here, let me know what do you like the best.

rebased onto 7595cdf
2 years ago

Build succeeded.

Do we really need that string before the actual error message?

When I look at the error message, it makes sense for me to keep it implemented the way you did it in this PR.

LGTM

Commit 940d971 fixes this pull-request

Pull-Request has been merged by praiskup
2 years ago
Metadata
No Tags
Changes Summary 7
+7 -0
file changed
beaker-tests/Sanity/copr-cli-basic-operations/runtest.sh
+21 -0
file changed
frontend/coprs_frontend/coprs/exceptions.py
+2 -2
file changed
frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_modules.py
+2 -1
file changed
frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_packages.py
+4 -4
file changed
frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_project_chroots.py
+6 -5
file changed
frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py
+13 -1
file changed
python/copr/v3/exceptions.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.