Implement Composer group repositories #14
Conversation
|
Hey @fjmilens3 ! I just gave your branch a shot. I have composer (group) now in the recipes, but I can not select it. Not sure if I'm to early with testing it, but just wanted to let you know 🙂 |
|
@TheBay0r, that concerns me somewhat because I'm able to select the composer (group) repo and create one without an issue. A couple of questions:
|
|
@fjmilens3, you are right. I was able to create a grouped repository now. I decided to combine When I try to do a composer install using the grouped repository only, I retrieve a NullPointerException. Accessing the same package via composer-hosted works |
@TheBay0r, this is a problem caused by the hackery I did in the proxy implementation to look up the provider JSON for a particular vendor and project in order to obtain the dist url. This probably wouldn't happen if you had your hosted repo before your proxy repo in the configuration, but it's still something we need to fix because it manifests a larger issue. The short version is that we're not robustly dealing with certain download situations in proxy (where a file that does not exist is requested, but it still checks upstream to reference any existing provider JSON first). For some background, Nexus Repo does group merge in two ways. For metadata, indices, and the like, it has to fetch all upstream responses from the member repos and merge them together. For actual content, we basically just hit each repository in succession until we find something and return that as the result. (You can see https://github.com/sonatype/nexus-public/blob/ffc3d00c05778ee415bfa69135d982bac2f38450/components/nexus-repository/src/main/java/org/sonatype/nexus/repository/group/GroupHandler.java and subclasses as the crown jewel of this particular dynasty.) In this case, you're running into a quirk of the Composer implementation I've done. The merge for all the metadata/indices works just fine, but when you're to fetching the zip archive, the code in proxy goes and tries to find the provider JSON to obtain the url indirectly in case it hasn't been stored yet or needs updated (this is largely a result of how the proxy support works in Nexus Repo). In that case, we're not dealing with the missing provider content gracefully, and the result is the stack trace you see before you. I have a pretty good idea of how to fix this as some other formats (npm) end up with similar problems, fortunately, and as today is our "improvement day" I think I can get something together quickly. I can't thank you enough for how involved you've been in contributing feedback and testing, it counts a lot to know what we do is valued! |
|
@fjmilens3 I'm sry that it took me so long to have another look at it. And I'm happy when my little testing helps you. Wish I could do even more, it is a great plugin 🙂 Downloading the |
| if (successfulResponses.isEmpty()) { | ||
| return notFoundResponse(context); | ||
| } | ||
| List<Payload> payloads = successfulResponses.stream().map(Response::getPayload).collect(Collectors.toList()); |
There was a problem hiding this comment.
Question: Can this map not be appended to the filter above? then you can still check for empty list, else merge and respond ok
There was a problem hiding this comment.
Question: Can this map not be appended to the filter above? then you can still check for empty list, else merge and respond ok
We can do that, mostly just following the convention from earlier formats where we'd filter by status, check, and then process the payloads. Particularly as we keep trying to open source formats, keeping things idiomatically consistent for the same operations could make it easier for people to read the code and contribute. Changed in 6d08022.
| if (successfulResponses.isEmpty()) { | ||
| return notFoundResponse(context); | ||
| } | ||
| List<Payload> payloads = successfulResponses.stream().map(Response::getPayload).collect(Collectors.toList()); |
There was a problem hiding this comment.
Same question applies here
Same answer. :) Done in 8b43ec5.
| @Nonnull final GroupHandler.DispatchedRepositories dispatched) | ||
| throws Exception | ||
| { | ||
| Repository repository = context.getRepository(); |
There was a problem hiding this comment.
Suggestion: This is a repeat of ComposerGroupPackagesJsonHandler#doGet, maybe you could create a common method and pass in a lambda which is the merge
There was a problem hiding this comment.
Suggestion: This is a repeat of ComposerGroupPackagesJsonHandler#doGet, maybe you could create a common method and pass in a lambda which is the merge
The lambda idea isn't my favorite for two reasons:
- We inject these into the
Recipes so that's not as straightforward as you might think. - Lambdas are nice for smaller bits of code such as filtering a collection, but their relative anonymity in a larger codebase can make it harder to find all the pieces you're looking for.
What do you think of 8f727aa instead?
| */ | ||
| public Content mergePackagesJson(final Repository repository, final List<Payload> payloads) throws IOException { | ||
| Set<String> names = new HashSet<>(); | ||
| for (Payload payload : payloads) { |
There was a problem hiding this comment.
Trivial: I think you could consider using functional approach here
There was a problem hiding this comment.
Trivial: I think you could consider using functional approach here
The reason I didn't do that was because we'd end up having to go the route of using UncheckedIOException because of the difficulty of throwing checked exceptions from within the lambdas in the stream.
| String time = now.withZone(DateTimeZone.UTC).toString(timeFormatter); | ||
|
|
||
| // TODO: Make this more robust, right now it makes a lot of assumptions and doesn't deal with bad things well, | ||
| // can probably consolidate this with the handling for rewrites for proxy (or at least make it more rational). |
There was a problem hiding this comment.
Suggestion: If possible I would consider breaking it down so it would be more manageable for someone else to visit
There was a problem hiding this comment.
I would at least break out the individual concerns i.e. buildDistInfo, buildPackageInfo
There was a problem hiding this comment.
I would at least break out the individual concerns i.e. buildDistInfo, buildPackageInfo
Done, also consolidated some near-duplicate code in 5ae0913.
| } | ||
|
|
||
| @Override | ||
| protected Response doGet(@Nonnull final Context context, |
There was a problem hiding this comment.
Trivial: I can't remember exactly but I thought we agreed that everything was considered non-null unless marked Nullable?
There was a problem hiding this comment.
Trivial: I can't remember exactly but I thought we agreed that everything was considered non-null unless marked Nullable?
We did, but the superclass defines @Nonnull so IDEA complains that the annotation has been removed (as analysis of the code would think we have a lesser restriction on nullability here than in the parent class). Until we take that out I'd rather do this than get warnings from IDEs or from static analysis.
| GroupFacet groupFacet = repository.facet(GroupFacet.class); | ||
| Map<Repository, Response> responses = getAll(context, groupFacet.members(), dispatched); | ||
| List<Response> successfulResponses = responses.values().stream() | ||
| .filter(response -> response.getStatus().getCode() == HttpStatus.OK && response.getPayload() != null) |
There was a problem hiding this comment.
Are you stripping out the conditional headers somewhere? i.e. if someone passes an if-not-modified to the group endpoint will we then pass that to the member repositories possibly resulting in a NOT_MODIFIED response?
There was a problem hiding this comment.
Are you stripping out the conditional headers somewhere?
Good catch, done in 788b51a.
| GroupFacet groupFacet = repository.facet(GroupFacet.class); | ||
| Map<Repository, Response> responses = getAll(context, groupFacet.members(), dispatched); | ||
| List<Response> successfulResponses = responses.values().stream() | ||
| .filter(response -> response.getStatus().getCode() == HttpStatus.OK && response.getPayload() != null) |
There was a problem hiding this comment.
Similar question to the packages handler about conditional gets
There was a problem hiding this comment.
Similar question to the packages handler about conditional gets
Done in 788b51a.
|
|
||
| builder.route(packagesMatcher() | ||
| .handler(timingHandler) | ||
| .handler(assetKindHandler.rcurry(AssetKind.PACKAGES)) |
There was a problem hiding this comment.
Trivial: no reason not to statically import these asset kinds.
There was a problem hiding this comment.
Trivial: no reason not to statically import these asset kinds.
I left them without static imports in the other recipes; to me it does provide a bit of useful context, but I'm not averse to changing them as part of a tidying PR at some point.
| .putString(time, StandardCharsets.UTF_8) | ||
| .hash() | ||
| .asInt()); | ||
| pkg.put(UID_KEY, Integer.toUnsignedLong( |
There was a problem hiding this comment.
Question: What was the reason for this change?
There was a problem hiding this comment.
Question: What was the reason for this change?
It's somewhat of a long story, but basically I didn't want to have negative values for UIDs.
| newPackageInfo.put(TIME_KEY, time); | ||
| newPackageInfo.put(UID_KEY, Integer.toUnsignedLong( | ||
| Hashing.md5().newHasher() | ||
| .putString(packageName, StandardCharsets.UTF_8) |
There was a problem hiding this comment.
Minor: As we do this twice a utility method that takes var args would make it clearer i.e. calculateMd5HashInteger(String... input);
There was a problem hiding this comment.
Minor: As we do this twice a utility method that takes var args would make it clearer i.e. calculateMd5HashInteger(String... input);
Consolidated to a single usage as part of 5ae0913.
|
|
||
| Map<String, Object> newDistInfo = new LinkedHashMap<>(); | ||
| newDistInfo.put(URL_KEY, String | ||
| .format(REWRITE_URL, repository.getUrl(), packageName, packageVersion, packageName.replace('/', '-'), |
There was a problem hiding this comment.
Minor: Although simple this would make a good util method to make refactoring easier in the future.
There was a problem hiding this comment.
Minor: Although simple this would make a good util method to make refactoring easier in the future.
Related change in 88f058a (also makes the output between proxy, hosted, and group more consistent for dist portions).
| String time = now.withZone(DateTimeZone.UTC).toString(timeFormatter); | ||
|
|
||
| // TODO: Make this more robust, right now it makes a lot of assumptions and doesn't deal with bad things well, | ||
| // can probably consolidate this with the handling for rewrites for proxy (or at least make it more rational). |
There was a problem hiding this comment.
I would at least break out the individual concerns i.e. buildDistInfo, buildPackageInfo
| } | ||
| } | ||
|
|
||
| return new Content(new StringPayload(mapper.writeValueAsString(Collections.singletonMap(PACKAGES_KEY, packages)), |
There was a problem hiding this comment.
Trivial: Collections doesn't provide any additional information here you could safely statically import it (i.e. In fact a map isn't strictly a "Collection" ... I wonder why they put the method there hmm).
There was a problem hiding this comment.
Trivial: Collections doesn't provide any additional information here you could safely statically import it (i.e. In fact a map isn't strictly a "Collection" ... I wonder why they put the method there hmm).
Done in 70805ad.
| return super.fetch(context, stale); | ||
| } | ||
| catch (NonResolvableProviderJsonException e) { | ||
| log.debug("Composer provider URL not resolvable: {}", e.getMessage()); |
There was a problem hiding this comment.
Question: What's the reason for this being debug and not error?
There was a problem hiding this comment.
Question: What's the reason for this being debug and not error?
Because it's not always an error, for example in the case that a proxy repo is being queried before a hosted repo that does contain an artifact; in that case the exception path is actually quite common, much like the npm implementation (in that it has to go remote to see if there's even a file containing the download url, and we have to have a way to indicate that within the confines of the formats framework we have).
See 95a20b5 and the comments around #14 (comment) and #14 (comment) for details.
|
I'm just about to build that branch and will test it this week. I'll let you know how it looks as soon as possible 🙂 |
|
@fjmilens3 it looks like there is some regression. 🤔 The composer-lock is missing a couple of informations now, that were there in the composer-hosted branch still. (Although the information is coming from composer-proxy). I also tried just using the composer-proxy without the composer-group feature as well. As you can see as an example for phpunit in the screenshot it is missing the information of the binary, which causes the Also the What is also a little bit concerning is, that we lost the
Hope it is readable enough for you. If you have any questions, just let me know. I even deployed latest master again to verify that this information wasn't missing after adding the composer-hosted feature. |
|
@TheBay0r, I've taken a quick look at this and made what I think are the necessary changes, but I have not tested it beyond unit testing. I'll probably try and take a look at this for further testing sometime next week as we're traveling this weekend, but you may end up getting to it before I do (and probably have more context than myself by far, in that I'd trust you more than I'd trust myself to find things wrong with it). As far as what happened here, as part of working on group I tried to be more consistent in terms of how we handled different fields in building and merging JSON, and the minimal subset that I chose was too minimal to support some of these additional features; when I do test this I'm usually testing against very trivial Also note that I added your name to our contributors list on b3e9b80 because of all the QA work you've done on this project over these months; if you want me to take that out I will, but I'd like to recognize your support if you're okay with that. |
| @@ -23,4 +23,4 @@ Sonatype internal people, in alphabetical order by username: | |||
|
|
|||
| External contributors: | |||
|
|
|||
|  | |||
| * [@TheBay0r](https://github.com/TheBay0r) (Stefan Schacherl) | |||
There was a problem hiding this comment.
That's a real honor, thank you :)
|
I can verify that the regression issue is seems to be resolved, although I currently have problems building one of our containers. I'll have to take a deeper look and will let you know if it is related to this branch. Something to clarify in the meanwhile. I understood you added the missing fields to composer-hosted as well. But when I have a look at one of our private packages, it doesn't look like it. See screenshot: |
|
Okay… it seems to be that the failing build is related to the "target-dir" property still missing the JSON.
|
@TheBay0r, I will take a look and test some hosted uploads myself to see what's going on. The changes only affect newly-deployed packages, was this one that you deployed after these changes or one that you had around previously in your Nexus storage? |
I already thought that question would come up, so I tested a package that was there before and also a package I just uploaded. Although I am not sure if it might interfere because I just replaced an existing version? |
It shouldn't, if it did that, then it's a bug (my first thought would center on whether or not we're correctly handling update events in terms of regenerating the metadata files). For what it's worth, I've tried it locally with a new upload and it seems to be working correctly (I've also added a few more fields to be stored), so I'm wondering if there is something more going on here. Could you put together a sample |
|
@fjmilens3 I just tried a few more things. Uploading a completely new version of the package -> Downloading JSON from But I still have to have a look at fcbc942 because those fields were obviously still missing. Providing you with a full composer.json might take me until Monday |
|
I guess one of the best composer.json you can find is actually the one from the composer repo itself. I just extendid it slightly by adding the repository field and adding an additional way for scripts. Also I removed the names. But I guess this covers most of the fields 🤔 {
"name": "composer/composer",
"description": "Composer helps you declare, manage and install dependencies of PHP projects, ensuring you have the right stack everywhere.",
"keywords": ["package", "dependency", "autoload"],
"homepage": "https://getcomposer.org/",
"type": "library",
"license": "MIT",
"authors": [
{
"name": "Not Person",
"email": "not.person@examplehomepagenotvalid.it",
"homepage": "http://www.examplehomepagenotvalid.it"
},
{
"name": "Another Not Person",
"email": "another@examplehomepagenotvalid.it",
"homepage": "http://www.examplehomepagenotvalid.it"
}
],
"support": {
"irc": "irc://irc.examplehomepagenotvalid.it/composer",
"issues": "https://github.com/composer/composer/issues"
},
"repositories": [
{
"type": "git",
"url": "https://github.com/foobar/intermediate.git",
"no-api": true
}
],
"require": {
"php": "^5.3.2 || ^7.0",
"justinrainbow/json-schema": "^3.0 || ^4.0 || ^5.0",
"composer/ca-bundle": "^1.0",
"composer/semver": "^1.0",
"composer/spdx-licenses": "^1.2",
"composer/xdebug-handler": "^1.1",
"seld/jsonlint": "^1.4",
"symfony/console": "^2.7 || ^3.0 || ^4.0",
"symfony/finder": "^2.7 || ^3.0 || ^4.0",
"symfony/process": "^2.7 || ^3.0 || ^4.0",
"symfony/filesystem": "^2.7 || ^3.0 || ^4.0",
"seld/phar-utils": "^1.0",
"psr/log": "^1.0"
},
"require-dev": {
"phpunit/phpunit": "^4.8.35 || ^5.7",
"phpunit/phpunit-mock-objects": "^2.3 || ^3.0"
},
"conflict": {
"symfony/console": "2.8.38"
},
"config": {
"platform": {
"php": "5.3.9"
}
},
"suggest": {
"ext-zip": "Enabling the zip extension allows you to unzip archives",
"ext-zlib": "Allow gzip compression of HTTP requests",
"ext-openssl": "Enabling the openssl extension allows you to access https URLs for repositories and packages"
},
"autoload": {
"psr-4": { "Composer\\": "src/Composer" }
},
"autoload-dev": {
"psr-4": { "Composer\\Test\\": "tests/Composer/Test" }
},
"bin": ["bin/composer"],
"extra": {
"branch-alias": {
"dev-master": "1.7-dev"
}
},
"scripts": {
"test": "phpunit",
"custom-cmd": [
"chmod -R 777 *"
]
}
}
|
|
Also had a look at fcbc942. It looks very good. The only thing is, that the .json generated in nexus is only updated when a new version is uploaded, but not when a existing version should be replaced 🤔 |
| newPackageInfo.put(EXTRA_KEY, versionInfo.get(EXTRA_KEY)); | ||
| } | ||
| if (versionInfo.containsKey(DESCRIPTION_KEY)) { | ||
| newPackageInfo.put(DESCRIPTION_KEY, versionInfo.get(DESCRIPTION_KEY)); |
There was a problem hiding this comment.
Looks like this is a duplicate for line 347-349
There was a problem hiding this comment.
Looks like this is a duplicate for line 347-349
Good catch! Removed in 9f29f63.
Thanks for getting that together, I ran it through once and it looks like we're in good shape. It did make me realize that I'm probably not handling merge correctly with respect to the
I appreciate you retesting those changes; at this point I'm going to merge this PR in as it should be sufficient for group.
I want to do some more investigation on this as time permits, for now I'm breaking it out into #20 so we can discuss it more there; whatever is going on here is more related to hosted than to group so should probably be worked separately. Thanks for all your help on this! |
|
Thanks for the contribution! Unfortunately we can't verify if the committer(s), Frederick John Milens III fmilens@sonatype.com, signed the CLA because they have not associated their commits with their GitHub user. Please follow these instructions to associate your commits with your GitHub user. Then sign the Sonatype Contributor License Agreement and this Pull Request will be revalidated. |
|
Thanks for the contribution! Unfortunately we can't verify if the committer(s), Frederick John Milens III fmilens@sonatype.com, signed the CLA because they have not associated their commits with their GitHub user. Please follow these instructions to associate your commits with your GitHub user. Then sign the Sonatype Contributor License Agreement and this Pull Request will be revalidated. |
Fixes #8 by adding basic support for merging (flattened) packages.json files and provider JSON files, along with the standard group handler for obtaining the correct zip file based on repository ordering.