Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 15 additions & 5 deletions services/maven-metadata/maven-metadata.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ const { BaseXmlService } = require('..')

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}).required()

const schema = Joi.object({
metadata: Joi.object({
versioning: Joi.object({
versions: Joi.object({
version: Joi.array().items(Joi.string().required()).single().required(),
}).required(),
latest: Joi.string().required(),
release: Joi.string().required(),
Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}).required(),
}).required(),
}).required()
Expand All @@ -35,6 +35,7 @@ module.exports = class MavenMetadata extends BaseXmlService {
queryParams: {
metadataUrl:
'https://repo1.maven.org/maven2/com/google/code/gson/gson/maven-metadata.xml',
latestOrRelease: 'latest',
},
staticPreview: renderVersionBadge({ version: '2.8.5' }),
},
Expand All @@ -52,10 +53,19 @@ module.exports = class MavenMetadata extends BaseXmlService {
})
}

async handle(_namedParams, { metadataUrl }) {
async handle(_namedParams, { metadataUrl, latestOrRelease }) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }) {

const data = await this.fetch({ metadataUrl })
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,
})
Comment on lines +58 to 69
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
28 changes: 28 additions & 0 deletions services/maven-metadata/maven-metadata.tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,34 @@ t.create('version ending with zero')
)
.expectBadge({ label: 'maven', message: 'v1.30' })

t.create('release version is handled properly')
.get(
'/v.json?metadataUrl=https://repo1.maven.org/maven2/mocked-group-id/mocked-artifact-id/maven-metadata.xml&latestOrRelease=release'
)
.intercept(nock =>
nock('https://repo1.maven.org/maven2')
.get('/mocked-group-id/mocked-artifact-id/maven-metadata.xml')
.reply(
200,
`
<metadata>
<groupId>mocked-group-id</groupId>
<artifactId>mocked-artifact-id</artifactId>
<versioning>
<latest>1.30</latest>
<release>1.29</release>
<versions>
<version>1.29</version>
<version>1.30</version>
</versions>
<lastUpdated>20190902002617</lastUpdated>
</versioning>
</metadata>
`
)
)
.expectBadge({ label: 'maven', message: 'v1.29' })

t.create('invalid maven-metadata.xml uri')
.get(
'/v.json?metadataUrl=https://repo1.maven.org/maven2/com/google/code/gson/gson/foobar.xml'
Expand Down