Make error message on invalid metadata friendlier

[alanbato]

Fixes #2668

I can't get the tests working due to #1536, so I'm 100% sure these changes work. 😅

Pinging @di for opinions on the message formatting, etc :)

[alanbato]

So the changes work, but now several tests fail expecting the old messages.
Also, I couldn't get the humanized names working, it's still showing them as version, name, and metadata_version, etc. Do we have them stored in the correct format somewhere?

[di]

Also, I couldn't get the humanized names working, it's still showing them as version, name, and metadata_version, etc. Do we have them stored in the correct format somewhere?

Nope, we don't. The mapping comes from the pkginfo project.

I think the best way to add that would be to include them as labels to the fields of the MetadataForm, then you could use it in the error message like field.label, and it shouldn't disrupt anything else about that form.

[alanbato]

Apparently field.label is an object which string representation has the HTML included, so field.label.text is needed for our purpose instead. I'll be waiting on the resolution of #2772 so fixing the tests is a little bit easier for me

[di]

@alanbato Just merged #2772 so you should be unblocked now.

[alanbato]

Got it, I'm changing the expected error messages on the tests right now.

[alanbato]

Hey @di, I've only 5 errors left to fix, and all of them have to do with errors related to the __all__ field_name, my assumption is that it means a field is missing from the form. Therefore I'm getting a KeyError trying to get the __all__ field from the form to make the error message.

I'm not sure how to proceed, should we write a different type error message for those cases? And not one like the x is an invalid value for y.
Please advise 😛

[di]

@alanbato Can you push what you've got so far so I can see the test failures? The latest build I'm seeing is this one.

[alanbato]

@di I did indeed forget to push my last changes. Now I pushed them, but I accidentally the commits. I tried to do the fetch/rebase combo to fix it to no avail. Do you happen to know how can I fix it?

[di]

@alanbato Coincidentally we have an open PR with some documentation updates for just this sort of thing, take a look at https://github.com/pypa/warehouse/pull/2760/files and see if that helps!

[alanbato]

It worked! Thank you. Funny how running those same commands in different order didn't help haha.

Now the remaining failing tests should be the ones I told you about :)

[di]

@alanbato The first two errors just seem to be that the tests haven't been updated to the new messages.

The ones concerning __all__ are because we're using __all__ to hold validation errors about the entire form, i.e. we're manually putting errors in form.errors["__all__"]. Since there isn't an actual __all__ field, your calls to form[field_name] are failing.

This means we'll need to raise a different error message depending on whether the field_name is in the form or not. Something like:

if field_name in form:
    raise _exc_with_message(
        HTTPBadRequest,
        "{value!r} is an invalid value for {field},".format(
            value=form[field_name].data,
            field=form[field_name].label.text) +
        "see https://packaging.python.org/specifications/core-metadata/",
    )
else:
    raise _exc_with_message(
        HTTPBadRequest,
        "Error: {}".format(form.errors[field_name][0])
    )

(note: I haven't tested that the above actually works)

[alanbato]

Thanks @di, I suspected as much, and thought of doing something similar to handle it (although a little bit less elegant haha).

All tests are passing locally, so I think this PR should be good to go :)

Unless you or any other maintaner say otherwise, of course 😅 haha

[di]

Could you remove the newlines from this error message?

[alanbato]

Removing the newline made the line longer than 80 characters, should I break it again?

[di]

By newlines, I mean the "\n" characters you have in this string.

[alanbato]

Totally my bad, sorry! I just removed the \ns 😅

[di]

Since these errors are not specific to a single field, it doesn't really make sense to include a link to the Core Metadata spec here.

[di]

This field (and a number of others) don't correspond to actual core metadata fields as specified at https://packaging.python.org/specifications/core-metadata/.

I think it would be somewhat confusing if these fields failed to validate and returned an error message that looks like it's referring to a core metadata field (including linking to the spec) but that field doesn't actually exist.

I think we should probably drop the label attribute from any of these that are not in the spec, and update the error message logic to only raise the "new" error message (which links to the spec) if the field has a label.

[alanbato]

Mmm, so do you think a good idea would be to change the code to something like this?

# Check both if the field is present or it has a label to show the 'new' error
if field_name in form and form[field_name].label:
    raise _exc_with_message(
        HTTPBadRequest,
        "{value!r} is an invalid value for {field},".format(
            value=form[field_name].data,
            field=form[field_name].label.text) +
        "see https://packaging.python.org/specifications/core-metadata/",
    )
else:
   # Fall back to the 'old' error in a more friendly manner
    raise _exc_with_message(
        HTTPBadRequest,
        "Error: {}".format(form.errors[field_name][0])
    )

[di]

The only issue with that is that it omits the field name that the original message had. I think there's three cases to consider here:

• the field name is in the form and has a label (it's a core metadata field) -> new error with link to spec
• the field name is in the form but doesn't have a label (it's one of these) -> original error
• the field name isn't in the form (it's __all__) -> new error without field name or link to spec

[di]

I think you'll need to add something before "see" here, as there isn't a space between it and the error string. I'd recommend changing this to ", see".

Also, it looks like there are still newlines in the tests, as some are failing.

[alanbato]

Yes, I'm changing the tests, I'm going to add those fixes to the test to the same commit as to stop the commit spamming I'm already doing haha.

As to the "see" part, I do have a space after the {}

Fixed

[di]

@alanbato Your changes look good, thanks! Looks like you'll need to add a test to cover the case where the field name is in the form, but it doesn't have a label.

Take a look at the coverage report here.

[alanbato]

Thanks @di! I'm currently working on that test, and the only fields that fit that criteria and are also validated are the sha256_digest and the blake2_digest.
So as I wrote the test for one of them I found out the new/full error was being printed, because for some reason they do have a field.label.text value and I can't find where they are defined. :/

[di]

Sorry about that @alanbato, looks like I gave you some bad advice. Seems that by default fields will always have a label:

>>> import wtforms
>>> class Test(wtforms.Form)
...     field = wtforms.fields.StringField()
...
>>> t = Test()
>>> t.field.label
Label('field', 'Field')
>>> t.field.label.text
'Field'

Looks like we can use description instead though:

>>> class Test(wtforms.Form):
...     field_no_desc = wtforms.fields.StringField()
...     field_with_desc = wtforms.fields.StringField(description="Foobar")
...
>>> t = Test()
>>> t.field_no_desc.description
''
>>> t.field_with_desc.description
'Foobar'

[alanbato]

That's okay, I tried to look into it but couldn't get to a working experimento to validate that same hypothesis, haha.
@di I changed every label to description and fixed the tests/added coverage :)

[di]

Thanks @alanbato!