Modify

Opened 14 months ago

Last modified 5 weeks ago

#21423 new enhancement

[PATCH][RFC] Document validator test identification codes

Reported by: gaben Owned by: gaben
Priority: normal Milestone: 22.11
Component: Wiki content Version:
Keywords: Cc:

Description

The core validator codes documented in the OsmValidator.java file, but validator plugins also can have one. If a collision happens, that's... not good?

If I understood correctly, the codes used in validator tests, where autofix possible, and fix carried out by identifying matching error codes among the others.

I think a dedicated wiki page with all the codes would be a solution. From there, plugin developers could choose an unused id for their application and update the list with it.

Probably not an issue yet, but who knows. What do you think?

Attachments (2)

21423.patch (48.8 KB) - added by taylor.smock 4 months ago.
Ensure error codes are unique via a (currently) optional method, errorCodes. The optional method should (eventually) become a hard requirement.
21423.2.patch (10.8 KB) - added by taylor.smock 3 months ago.
Prepend hashcode of tester class name to error code. Contains compatibility code (not well tested), new errors are saved with the new code, old errors will be migrated starting 2024 (to allow users to move between tested/latest versions)

Download all attachments as: .zip

Change History (27)

comment:1 Changed 14 months ago by Don-vip

Owner: changed from team to gaben

Feel free to document this on DevelopersGuide/DevelopingPlugins, it's a wiki :)

comment:2 Changed 14 months ago by taylor.smock

@Gaben: I've handled this in my plugins by choosing some number (e.g. 12), then multiplying it by some large number (e.g. 100,000), and then starting the numbering there. I think all of the JOSM core validations are < 20,000 (and most are < 10,000).

Exceptions: If I intend to move the validation to JOSM core, I will use a smaller number. But it is easy enough to change it (if you don't mind dropping all the ignore values).

comment:3 Changed 14 months ago by gaben

I'm thinking not only about core<->plugin, but plugin<->plugin. It has the same effect.

Also JOSM core can't detect validator ID collisions yet?

comment:4 Changed 11 months ago by gaben

Added a note about collisions https://josm.openstreetmap.de/wiki/DevelopersGuide/DevelopingPlugins?action=diff&version=111.

I will look around how is it possible to implement a simple collision detection.

comment:5 in reply to:  4 ; Changed 11 months ago by taylor.smock

Replying to gaben:

Added a note about collisions https://josm.openstreetmap.de/wiki/DevelopersGuide/DevelopingPlugins?action=diff&version=111.

I will look around how is it possible to implement a simple collision detection.

Technically possible, but you are probably going to need to add a method which returns a list to the base validator class. You could _try_ looking at static ints, but that will only take you so far.

I've got a GitLab job that looks for plugin incompatibilities that you can use to figure out who you need to notify about the change.

comment:6 in reply to:  5 ; Changed 5 months ago by gaben

Replying to taylor.smock:

Technically possible, but you are probably going to need to add a method which returns a list to the base validator class.

The codes are defined in the child classes, not in the OsmValidator.java so I don't see an easy way to extract them :/

comment:7 in reply to:  6 Changed 5 months ago by taylor.smock

Replying to gaben:

Replying to taylor.smock:

Technically possible, but you are probably going to need to add a method which returns a list to the base validator class.

The codes are defined in the child classes, not in the OsmValidator.java so I don't see an easy way to extract them :/

There really isn't. What you would have to do is:

  1. Add a new mandatory method in the parent to get all error codes that the check will use. All downstream checks would be broken until they implement said method, if we use that method in core.
  2. Load all plugins into JOSM
  3. Run a class which iterates through all validators and calls said method, and adds it to a map/list
  4. ???

Like I said, it would be technically possible to do this, but most plugins are dormant at this point (or they are maintained by the JOSM core team).

comment:8 Changed 5 months ago by gaben

See [18506:18507], as the current situation is error-prone. Do you know roughly how many plugins are affected?

comment:9 in reply to:  8 Changed 5 months ago by taylor.smock

Replying to gaben:

See [18506:18507], as the current situation is error-prone. Do you know roughly how many plugins are affected?

I honestly don't know. I know the mapwithai plugin could be impacted, but there is nothing in JOSM svn that would be impacted.

comment:10 Changed 4 months ago by taylor.smock

Well, I just found conflicting error codes in pt_assistant.

comment:11 Changed 4 months ago by gaben

Yeah another example of a conflict. Some kind of conflict detector needed in my opinion.

comment:12 Changed 4 months ago by taylor.smock

I'm doing some work on that right now. The initial version will be opt-in, but may eventually become mandatory.

comment:13 Changed 4 months ago by gaben

What's the plan on code level? Yesterday I had a look on the current implementation.

comment:14 in reply to:  13 Changed 4 months ago by taylor.smock

Replying to gaben:

What's the plan on code level? Yesterday I had a look on the current implementation.

It is really stupid, but it is the only thing I can think of.

  1. Each test will specify the error codes it uses. Right now, this is optional (AKA there is a default implementation, although the base Test class is not abstract, so there is another change (there may be binary compatibility issues, I don't know yet)).
  2. The various places where tests are run will check that the returned errors have error codes in that test's error code list.

Note that this will *not* guard against a test reusing an error code for different things (see r18507).

comment:15 Changed 4 months ago by gaben

Yesterday I had the same idea. Yeah, not the best solution but at least doable.

comment:16 Changed 4 months ago by GerdP

See #18230

Changed 4 months ago by taylor.smock

Attachment: 21423.patch added

Ensure error codes are unique via a (currently) optional method, errorCodes. The optional method should (eventually) become a hard requirement.

comment:17 Changed 4 months ago by taylor.smock

An additional quick comment on attachment:21423.patch:

I'm looking at creating a new method in OsmValidator to run enabled tests so that we aren't doing (effectively) the same thing in three different places, and that is where I'll add the check to ensure all test errors use a declared error code.

Last edited 4 months ago by taylor.smock (previous) (diff)

comment:18 Changed 4 months ago by taylor.smock

Priority: minornormal
Summary: Document validator test identification codes[PATCH][RFC] Document validator test identification codes
Type: taskenhancement

comment:19 Changed 3 months ago by taylor.smock

Milestone: 22.09

comment:20 Changed 3 months ago by stoecker

Hmm. When you really want to make this collision free I'd suggest another approach:

  • Use whatever HASH algorithm you want to hash the class name (which should be unique) and use that as base value.
  • Add 1 for each use case.

comment:21 Changed 3 months ago by stoecker

P.S. To reduce the impact I'd only do this for the plugins and leave the core as is. It's very unlikely, that the core values will be produced by a hash algorithm :-)

comment:22 in reply to:  20 Changed 3 months ago by taylor.smock

Replying to stoecker:

Hmm. When you really want to make this collision free I'd suggest another approach:

  • Use whatever HASH algorithm you want to hash the class name (which should be unique) and use that as base value.
  • Add 1 for each use case.

We could probably modify TestError.Builder to use the passed class along with the error code. The only problem here is reading old warnings and converting them to new codes.

But changing the builder is (essentially) a oneliner, unless we want exceptions for JOSM validators.
this.code = this.tester.getClass().getName().hashCode() + code instead of this.code = code. The hard part would be backwards compatibility with ignore lists.

We should probably still have something looking for duplicates though, since hashes can and will have collisions. Especially efficient ones.

Changed 3 months ago by taylor.smock

Attachment: 21423.2.patch added

Prepend hashcode of tester class name to error code. Contains compatibility code (not well tested), new errors are saved with the new code, old errors will be migrated starting 2024 (to allow users to move between tested/latest versions)

comment:23 Changed 3 months ago by taylor.smock

I'm going out of town for the next few weeks, so I'm uploading what I currently have for this ticket. It probably needs work and definitely needs tests.

comment:24 Changed 2 months ago by taylor.smock

Milestone: 22.0922.10

comment:25 Changed 5 weeks ago by taylor.smock

Milestone: 22.1022.11

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain gaben.
as The resolution will be set.
to The owner will be changed from gaben to the specified user.
The owner will change to gaben
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket
The owner will be changed from gaben to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.