[PATCH][RFC] Document validator test identification codes

[gaben]

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?

[Don-vip]

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

[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).

[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?

[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.

[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.

[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 :/

[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).

[gaben]

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

[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.

[taylor.smock]

Well, I just found conflicting error codes in pt_assistant.

[gaben]

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

[taylor.smock]

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

[gaben]

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

[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).

[gaben]

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

[GerdP]

See #18230

[taylor.smock]

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

[taylor.smock]

An additional quick comment on attachment:21423.patch​:

• I did not add a check at test runtime to ensure that tests only ever use a declared error code. This is because we are running tests in three separate areas :
  • ValidateAction#doValidate​
  • ValidatorCLI#processFile​
  • ValidateUploadHook#checkUpload​

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.

[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.

[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 :-)

[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.

[taylor.smock]

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)

[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.

[taylor.smock]

Milestone renamed

[taylor.smock]

Ticket retargeted after milestone closed

[taylor.smock]

Add tests

[taylor.smock]

In 18636/josm:

Fix #21423: Prevent error codes from clashing

This works by creating a unique code using the test class name. The new
format for ignore entries will look like UNIQUECODE_CODE_MESSAGE_STATE.
The switchover date for the new entries is set at 2024-01-01 in order to
give users sufficient time to update, such that if a user has multiple
installations of JOSM, all versions will be able to use the same ignore
list. The compatibility code is intended to be removed in 2025.

[GerdP]

I guess josm\core\nodist\data\multipolygon.osm needs changes to make unit tests work, as it contains hardcoded numbers of the expected error codes.

[gaben]

Just had a quick look and spotted since xxx. Does svn have precommit hooks?

[taylor.smock]

Replying to gaben:

Just had a quick look and spotted since xxx. Does svn have precommit hooks?

I did run it. But I probably ran into it not working on the changelist I was committing.

[taylor.smock]

Replying to GerdP:

I guess josm\core\nodist\data\multipolygon.osm needs changes to make unit tests work, as it contains hardcoded numbers of the expected error codes.

It doesn't need changes right now -- we'll need to update it around 2024-01-01 -- the code won't start writing the new unique ids until then, but it will understand the new codes.

EDIT: To clarify, it will understand new and old codes until we remove the translation code, which I'm currently intending to remove sometime in 2025.

[taylor.smock]

@GerdP: I've opened a new ticket for updating error codes. See #22654 -- I've assigned it to milestone:"24.01".

EDIT: Nevermind. The tests will continue to pass.

The only place where josm_error_codes is read is in ValidatorTestUtils, and it only compares it against the TestError#getCode return, which was not changed.

[stoecker]

Replying to gaben:

Just had a quick look and spotted since xxx. Does svn have precommit hooks?

It has. But sadly hooks aren't allow to change the commit itself otherwise the xxx-replacement would be automatic already.

[taylor.smock]

To add on, TortoiseSVN (apparently) has some way to have a pre-commit hook on the dev machine. Unfortunately, it is not a feature of SVN itself.

I should probably make a customization to since_xxx.py to take a changelist as an argument -- my IDE likes to have everything in a changelist, such that the "default" changelist is not null, but instead a changelist called Default.