Use release/latest fields of [mavenmetadata] instead of last version in the versions, add according query parameter #6594

CommanderTvis · 2021-06-07T19:14:40Z

The problem is that it is invalid to just take the last field from Maven metadata.

This PR solves it by taking latest field by default and release field if the new query parameter is specified so.

resolves #6188

shields-ci · 2021-06-07T19:15:34Z

	Messages
📖	✨ Thanks for your contribution to Shields, @CommanderTvis!

Generated by 🚫 dangerJS against d8f0c84

calebcartwright · 2021-06-08T00:09:37Z

hi @CommanderTvis 👋 thanks for the PR!

I've updated your PR title to follow our required conventions so that the tests will be run, and as you'll notice there are failures.

I have a few asks of you:

Could you please provide some more context on what you are proposing be changed, and why? We don't require PRs have a preceding issue, but PRs without an associated issue should provide sufficient information about the change
Please utilize the PR description and, if needed, comments to convey the additional information requested. The PR title should be a succinct summary (no need to enumerate every change, nor technical details in the title!)
Please review the test failure(s) and adjust the proposed changes accordingly. More details on the failure(s) can be found in CircleCI (e.g. https://app.circleci.com/pipelines/github/badges/shields/7087/workflows/cd8b8a44-c769-48a3-a26b-1b3af0392ce1/jobs/145824)

calebcartwright · 2021-06-08T00:10:27Z

Also cc @PyvesB for Java expertise given the Maven touch point

…n the versions, add according query parameter

CommanderTvis · 2021-06-08T13:25:40Z

@calebcartwright I've updated the description

calebcartwright · 2021-06-08T13:32:08Z

@calebcartwright I've updated the description

Thanks, though that still doesn't really answer my question. This is a badge that has been around and in use for a long time, so I'd ask for more information, examples, details about why you think it's invalid vs. just labeling it as such.

CommanderTvis · 2021-06-08T13:34:28Z

@calebcartwright I've updated the description

Thanks, though that still doesn't really answer my question. This is a badge that has been around and in use for a long time, so I'd ask for more information, examples, details about why you think it's invalid vs. just labeling it as such.

Here's my use case: https://img.shields.io/maven-metadata/v?label=Space&metadataUrl=https%3A%2F%2Fmaven.pkg.jetbrains.space%2Fmipt-npm%2Fp%2Fsci%2Fmaven%2Fspace%2Fkscience%2Fkmath-core%2Fmaven-metadata.xml

The metadata looks like this:

<metadata>
<groupId>space.kscience</groupId>
<artifactId>kmath-core</artifactId>
<versioning>
<latest>0.3.0-dev-12</latest>
<release>0.3.0-dev-12</release>
<versions>
<version>0.3.0-dev-12</version>
<version>0.3.0-dev-10</version>
<version>0.3.0-dev-8</version>
<version>0.3.0-dev-3</version>
<version>0.3.0-dev-2</version>
<version>0.2.1</version>
<version>0.2.0</version>
</versions>
<lastUpdated>20210522171951</lastUpdated>
</versioning>
</metadata>

The correct latest version is 0.3.0-dev-12, but the badge takes 0.2.0.

CommanderTvis · 2021-06-08T13:36:40Z

The specification of Maven metadata says nothing about the order of versions, but the latest field is really the latest version.

calebcartwright · 2021-06-08T15:21:14Z

Thanks @CommanderTvis! That's exactly what I was looking for.

This looks like another case of #6188 in that JetBrains, like Gradle, aren't sorting the versions like Maven Central. It seems a given that something needs to change, as the status quo seems to be more and more of a "Maven Metadata but really just for Maven Central", but I'm unsure of the potential impact on any existing badge usage (e.g. could this change result in a different/unexpected version value in the badge message for cases using different/older maven versions, different providers/hosts, etc.)

CommanderTvis · 2021-06-08T16:32:06Z

Thanks @CommanderTvis! That's exactly what I was looking for.

This looks like another case of #6188 in that JetBrains, like Gradle, aren't sorting the versions like Maven Central. It seems a given that something needs to change, as the status quo seems to be more and more of a "Maven Metadata but really just for Maven Central", but I'm unsure of the potential impact on any existing badge usage (e.g. could this change result in a different/unexpected version value in the badge message for cases using different/older maven versions, different providers/hosts, etc.)

For libraries published with Maven, nothing will change.

calebcartwright · 2021-06-08T22:55:17Z

For libraries published with Maven, nothing will change.

My point is that while the odds of impact may be minuscule, I don't think it's possible to claim there's a 0% risk of impacting existing badges because this badge can be used with any arbitrary xml file

We take backwards compatibility with our badges seriously, and that includes not breaking badge routes and trying to avoid server-side changes causing existing badges to display differently (different colors, different textual values, etc.). Sometimes such changes are unavoidable, but when they are (or could be) we do need to think through potential impacts and try to communicate the pending changes as proactively as possible.

It's pretty clear that different registries will handle the same maven information differently/contain different file contents for the same package, so I definitely think it's worth taking a moment to see if anyone (maintainer or community) is aware of any potential issues or impacts.

In the interim, please note that you can alternatively use our existing DynamicXML badge today to go ahead and get a working badge, e.g.

https://img.shields.io/badge/dynamic/xml?label=Space&query=//metadata/versioning/latest&url=https%3A%2F%2Fmaven.pkg.jetbrains.space%2Fmipt-npm%2Fp%2Fsci%2Fmaven%2Fspace%2Fkscience%2Fkmath-core%2Fmaven-metadata.xml

CommanderTvis · 2021-06-09T16:05:41Z

For libraries published with Maven, nothing will change.

My point is that while the odds of impact may be minuscule, I don't think it's possible to claim there's a 0% risk of impacting existing badges because this badge can be used with any arbitrary xml file

We take backwards compatibility with our badges seriously, and that includes not breaking badge routes and trying to avoid server-side changes causing existing badges to display differently (different colors, different textual values, etc.). Sometimes such changes are unavoidable, but when they are (or could be) we do need to think through potential impacts and try to communicate the pending changes as proactively as possible.

It's pretty clear that different registries will handle the same maven information differently/contain different file contents for the same package, so I definitely think it's worth taking a moment to see if anyone (maintainer or community) is aware of any potential issues or impacts.

In the interim, please note that you can alternatively use our existing DynamicXML badge today to go ahead and get a working badge, e.g.

https://img.shields.io/badge/dynamic/xml?label=Space&query=//metadata/versioning/latest&url=https%3A%2F%2Fmaven.pkg.jetbrains.space%2Fmipt-npm%2Fp%2Fsci%2Fmaven%2Fspace%2Fkscience%2Fkmath-core%2Fmaven-metadata.xml

Oh, that's a nice workaround. Thank you. Personally, I don't need it then.

PyvesB · 2021-06-13T08:47:20Z

Whilst I'm on board with the backwards compatibility considerations, in this case one could argue that taking the last item in a list of versions with no defined order is a bug.

In an ideal world, I would want us to collate all the requests for this badge for a day or so, and write a little script that compares the versions results of both approaches. We could then measure the impact and see if it would lead to unintended results in some cases. However, we don't store the requests made to Shields.io, if we wanted to do this we would have to use a much smaller time frame and tail the servers logs in real time.

calebcartwright · 2021-06-15T01:41:30Z

Whilst I'm on board with the backwards compatibility considerations, in this case one could argue that taking the last item in a list of versions with no defined order is a bug.

Thanks @PyvesB. I was curious whether there would be any well known cases where the latest and release elements simply wouldn't exist in the XML and if this change would in turn cause invalid response data badges. For example packages that had only published snapshots or prereleases, or maybe a system like Bintray or Nexus was known to elide those elements (for whatever odd reason). If both are always there, and based on the specification (for both v3 and v2) it seems like they should be, I'm struggling to follow why we were using the versions collection in the first place 🤷‍♂️

Took a bit of digging but looks like it was added in #853 not too terribly long ago (Paul ended up grabbing the commits on another PR to help the author get it across the finish line, but #853 was the initial implementation). Based on the dialog/details, or lack thereof, I assume this was indeed more likely to be an oversight vs. a deliberate need to avoid latest and release.

calebcartwright

Thanks again for this! The gist of the change looks good, though will need to tweak a few things to be more consistent with how Shields handles version badges.

Details left inline, one of the other version badges (e.g. clojars) may be a helpful reference as well

calebcartwright · 2021-06-15T01:48:58Z

services/maven-metadata/maven-metadata.service.js

@@ -7,14 +7,14 @@ const { BaseXmlService } = require('..')

 const queryParamSchema = Joi.object({
  metadataUrl: optionalUrl.required(),
+  latestOrRelease: Joi.string().valid('release', 'latest').optional(),


One of the many areas we strive for consistency within Shields (beyond the svg's themselves) are things like the badge route patterns and query parameter names for common functions.

For the latest vs. release function we use a query param name of include_prereleases, and the presence of the param providing the implicit boolean value.

calebcartwright · 2021-06-15T01:50:24Z

services/maven-metadata/maven-metadata.service.js

+      latest: Joi.string().required(),
+      release: Joi.string().required(),


Are we positive both of these elements will always be there? What does the xml look like for a package that has no releases/only snapshots? Based on the example you shared it seems like it the release element will still be present, just with the same value as latest. Is that correct?

calebcartwright · 2021-06-15T01:51:55Z

services/maven-metadata/maven-metadata.service.js

@@ -52,10 +53,19 @@ module.exports = class MavenMetadata extends BaseXmlService {
    })
  }

-  async handle(_namedParams, { metadataUrl }) {
+  async handle(_namedParams, { metadataUrl, latestOrRelease }) {


this would need to be changed accordingly (with rename in destructuring to satiate the linters):

Suggested change

async handle(_namedParams, { metadataUrl, latestOrRelease }) {

async handle(_namedParams, { metadataUrl, include_prereleases: includePrereleases }) {

calebcartwright · 2021-06-15T02:01:35Z

services/maven-metadata/maven-metadata.service.js

+    let v
+    if (
+      latestOrRelease === 'latest' ||
+      typeof latestOrRelease === 'undefined'
+    ) {
+      v = data.metadata.versioning.latest
+    } else if (latestOrRelease === 'release') {
+      v = data.metadata.versioning.release
+    }
    return renderVersionBadge({
-      version: data.metadata.versioning.versions.version.slice(-1)[0],
+      version: v,
    })


Just FYI, a fairly common pattern in our service classes is to utilize another function transform that handles the concern of extracting the target elements from the raw response, and if necessary mapping or transforming that. That's not necessary here due to the simplicity, but sharing for future awareness as it's easier to test and often a bit cleaner too.

We also default our version badges to showing the latest stable/release version (release here), with the include_prereleases query param available for users that want to display the absolute latest regardless of stable/release vs. prerelease. However, even in the default case if there are no "release" versions, we will still automatically fallback to a prerelease-type version vs. throwing an error.

If both the latest and release elements are always present and already reflecting that pattern, then we should be fine with a simple conditional. However, if release is sometimes absent then we'd need to account for that fallback pattern too

CommanderTvis · 2021-07-29T21:27:51Z

Closed the PR because I'm satisfied with the Dynamic XML workaround.

calebcartwright changed the title ~~Use release/latest fields of maven-metadata instead of last version in the versions, add according query parameter~~ Use release/latest fields of [maven-metadata] instead of last version in the versions, add according query parameter Jun 7, 2021

calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Jun 7, 2021

calebcartwright changed the title ~~Use release/latest fields of [maven-metadata] instead of last version in the versions, add according query parameter~~ Use release/latest fields of [mavenmetadata] instead of last version in the versions, add according query parameter Jun 8, 2021

Use release/latest fields of maven-metadata instead of last version i…

d8f0c84

…n the versions, add according query parameter

calebcartwright reviewed Jun 15, 2021

View reviewed changes

shields-cd temporarily deployed to shields-staging-pr-6594 June 15, 2021 03:29 Inactive

CommanderTvis closed this Jul 29, 2021

PyvesB mentioned this pull request Sep 11, 2021

[maven-metadata] Allow filtering for release versions #6995

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use release/latest fields of [mavenmetadata] instead of last version in the versions, add according query parameter #6594

Use release/latest fields of [mavenmetadata] instead of last version in the versions, add according query parameter #6594

CommanderTvis commented Jun 7, 2021 •

edited

shields-ci commented Jun 7, 2021 •

edited

calebcartwright commented Jun 8, 2021

calebcartwright commented Jun 8, 2021

CommanderTvis commented Jun 8, 2021

calebcartwright commented Jun 8, 2021

CommanderTvis commented Jun 8, 2021

CommanderTvis commented Jun 8, 2021 •

edited

calebcartwright commented Jun 8, 2021

CommanderTvis commented Jun 8, 2021

calebcartwright commented Jun 8, 2021

CommanderTvis commented Jun 9, 2021

PyvesB commented Jun 13, 2021

calebcartwright commented Jun 15, 2021

calebcartwright left a comment

calebcartwright Jun 15, 2021

calebcartwright Jun 15, 2021

calebcartwright Jun 15, 2021

calebcartwright Jun 15, 2021

CommanderTvis commented Jul 29, 2021

		latest: Joi.string().required(),
		release: Joi.string().required(),

	async handle(_namedParams, { metadataUrl, latestOrRelease }) {
	async handle(_namedParams, { metadataUrl, include_prereleases: includePrereleases }) {

Use release/latest fields of [mavenmetadata] instead of last version in the versions, add according query parameter #6594

Use release/latest fields of [mavenmetadata] instead of last version in the versions, add according query parameter #6594

Conversation

CommanderTvis commented Jun 7, 2021 • edited

shields-ci commented Jun 7, 2021 • edited

calebcartwright commented Jun 8, 2021

calebcartwright commented Jun 8, 2021

CommanderTvis commented Jun 8, 2021

calebcartwright commented Jun 8, 2021

CommanderTvis commented Jun 8, 2021

CommanderTvis commented Jun 8, 2021 • edited

calebcartwright commented Jun 8, 2021

CommanderTvis commented Jun 8, 2021

calebcartwright commented Jun 8, 2021

CommanderTvis commented Jun 9, 2021

PyvesB commented Jun 13, 2021

calebcartwright commented Jun 15, 2021

calebcartwright left a comment

Choose a reason for hiding this comment

calebcartwright Jun 15, 2021

Choose a reason for hiding this comment

calebcartwright Jun 15, 2021

Choose a reason for hiding this comment

calebcartwright Jun 15, 2021

Choose a reason for hiding this comment

calebcartwright Jun 15, 2021

Choose a reason for hiding this comment

CommanderTvis commented Jul 29, 2021

CommanderTvis commented Jun 7, 2021 •

edited

shields-ci commented Jun 7, 2021 •

edited

CommanderTvis commented Jun 8, 2021 •

edited