all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Thomas Lamprecht <t.lamprecht@proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-network 1/2] fix #7077: Improve error messages for ID length mismatch
Date: Fri, 6 Feb 2026 13:07:20 +0100	[thread overview]
Message-ID: <mp3ggxjqimualakem43bge5szrphz2dyat2wynowsujwl7wcbu@6pf4sbwxam6u> (raw)
In-Reply-To: <e68c8b69-4b9d-4cbe-90c8-99e16f8f4c48@proxmox.com>

On Fri, Feb 06, 2026 at 10:40:14AM +0100, Fiona Ebner wrote:
> Am 30.01.26 um 9:23 AM schrieb Arthur Bied-Charreton:
> > On Thu, Jan 29, 2026 at 06:38:50PM +0100, Thomas Lamprecht wrote:
> >> Am 21.01.26 um 11:34 schrieb Arthur Bied-Charreton:
> >>> Add explicit length checks to ID validation functions to provide clearer
> >>> error messages in case of length mismatches
> >>
> >> A few of these checks might be also done by setting the "maxLength" and/or the
> >> "minLength" property in the matching "register_standard_option" schema definition.
> >> Could you check that? Would be IMO better to reuse the schema capabillities, and
> >> that way, this limits would be also visible in the api schema directly.
> >>
> >>
> > Hey Thomas, thanks for the feedback, I did not think about that.
> 
> Not Thomas, but taking the freedom to respond :)
> 
> > After looking into this, it seems like we could get rid of most of the parse_*_id functions
> > (and probably others as well), but this would require changing PVE::JSONSchema::check_format
> 
> Do you mean check_prop() here?
> 
Yes I do, got the names confused -_- Thanks!

> > to check in the following order: 
> > 
> > `length -> pattern -> custom format fn`
> > 
> > as opposed to currently:
> > 
> > `custom format fn -> pattern -> length`
> > 
> > In the JSON Schema validation's current implementation, the length parameters are effectively ignored 
> > due to the pattern/format function being checked beforehand. Do you see any issue with changing the 
> > validation order?
> 
> AFAICS, changing the order does not change the execution flow. For any
> violation, add_error() is called and then the function returns. So for
> inputs with multiple violations, changing the order of checks just
> changes which error message is present. When messages about length are
> more telling to the user (and I think that's true in most cases), it is
> an improvement if they are preferred.
>
That is what I thought/observed as well.

> Do you have a good reason for also changing the order between the format
> fn and pattern? Are there even cases where both are used? If there are
> such cases, I guess it depends on the format fn implementation whether
> the error message from there is more telling than "value does not match
> the regex pattern". What do you think?
I do not, I wrongly operated under the assumption that format fns would 
always have the kind of error message we are trying to get rid of. It is
true that custom error messages are unlikely to be *less* telling than 
regex errors, the correct order is more 

length -> fn -> regex.

Thanks for catching that!

Now since a lot of the registered format functions only check values against 
their expected lengths and patterns, they could be fully replaced
by registering schemas with register_standard_option instead of custom
code. I will submit a v2 with these changes after I made sure that
removing the registered formats does not result in any regressions. Of
course if anyone already knows about some off the top of their head, I would be
thankful for any tips :).




  reply	other threads:[~2026-02-06 12:07 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-01-21 10:32 [pve-devel] [PATCH 0/5] fix #7077: Improve ID validation error messages Arthur Bied-Charreton
2026-01-21 10:32 ` [pve-devel] [PATCH pve-common] fix #7077: Improve error message for IDs shorter than 2 characters Arthur Bied-Charreton
2026-01-21 18:00   ` [pve-devel] applied: " Thomas Lamprecht
2026-01-21 10:32 ` [pve-devel] [PATCH pve-storage] fix #7077: Improve error message for IDS " Arthur Bied-Charreton
2026-01-21 10:32 ` [pve-devel] [PATCH proxmox] fix #7077: Improve error message for SDN IDs " Arthur Bied-Charreton
2026-01-21 10:32 ` [pve-devel] [PATCH pve-network 1/2] fix #7077: Improve error messages for ID length mismatch Arthur Bied-Charreton
2026-01-29 17:38   ` Thomas Lamprecht
2026-01-30  8:23     ` Arthur Bied-Charreton
2026-02-06  9:40       ` Fiona Ebner
2026-02-06 12:07         ` Arthur Bied-Charreton [this message]
2026-01-21 10:32 ` [pve-devel] [PATCH pve-network 2/2] fix #7077: Respect noerr flag in SDN ID validation Arthur Bied-Charreton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=mp3ggxjqimualakem43bge5szrphz2dyat2wynowsujwl7wcbu@6pf4sbwxam6u \
    --to=a.bied-charreton@proxmox.com \
    --cc=f.ebner@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal