I think it makes more sense here to return 400 if n is a garbage value rather than just ignore it. Just go into this block if n is set.

Sure, I can do that!
Keep in mind that atm. catalog has the same behavior, where it will not throw any error on a invalid n value (https://github.com/docker/distribution/blob/master/registry/handlers/catalog.go#L41-L44).

Should I make a PR to get them to match up?

I think that is the right behavior, but considering that API is not used by end clients, it's not super important.

We can possibly discuss the error handling of these in the distribution spec as well if it is too ambiguous. Handling bad input (n is not a number, last does not exist) could be clearly defined.

Lets get this merged first then I can get the other branch up-to-date and use the distribution spec in that as well.
Somewhat easy to do for n, but when last is wrong it is a different case (either the tag does not exists or the repository doesn't).

I currently have this wording for the spec, but feel unsure about the wording.

	// ErrorPaginationNumberInvalid is returned when the requested number
	// of results n is not a integer.
	ErrorPaginationNumberInvalid = errcode.Register(errGroup, errcode.ErrorDescriptor{
		Value:   "PAGINATION_NUMBER_INVALID",
		Message: "invalid number of results requested",
		Description: `This error may be returned when the requested
		"n" number of results is not an integer type`,
		HTTPStatusCode: http.StatusBadRequest,
	})

Would love some input, and maybe a suggestion for the last param. Else I will try to word it myself tomorrow :)

@dmcgowan fixed in latest commits, please check to ensure it is correct :)

Hoping a ping is OK,new Github notifications isn't nice @dmcgowan :|

I saw this, but wanted to cross reference these changes with the spec first before merging. Just went through now, the section is https://github.com/opencontainers/distribution-spec/blob/master/spec.md#tags-paginated

This error is the one that I wanted to double check. The spec says for last that Result set will include values lexically after last. In this case, the search should be done, but it can still continue even if last doesn't exist but entries exist lexically after it.

@jdolitsky do you mind offering an opinion here?

That makes perfect sense tbh. most have missed that it specified that. I sadly read it as after the index of the last string and values should be sorted lexically.
Seems like that should be the behavior :)

Just say the word and I will update it in this (and update the catalog PR after this is merged to do the same).

@dmcgowan @eyJhb I'm here on behalf of @jdolitsky
You're correct, the spec says that the /v2/<name>/tags/list endpoint should return a lexically-ordered list when the n query parameter is present, regardless of whether last is specified.

Paginated tag results can be retrieved by adding an n parameter to the request URL, declaring that the response SHOULD be limited to n results. Starting a paginated flow MAY begin as follows:

GET /v2/<name>/tags/list?n=<integer>
The above specifies that a tags response SHOULD be returned, from the start of the result set, ordered lexically, limiting the number of results to n. The response to such a request would look as follows:

https://github.com/opencontainers/distribution-spec/blob/master/spec.md#pagination

Usually the value of the last key would be acquired by by a Link header from an earlier response. Failing that, the registry should probably behave intelligently when the actual last value doesn't exist.

The current language in the spec is at the SHOULD level, but this will probably become a MUST by the time distribution-spec 1.0 is released.

What I gather from this is the following. last does not have to match a value in the tags, but should specify where in the tags we want to get our tags from legically (so this should be changed in this PR).

I am wondering if last should be ignored, if no n is specified? Below is from the doc.

The client can then issue the request with the above value from the Link header, receiving the values v3 and v4. Note that n may change on the second to last response or be fully omitted, depending on the server implementation.

Keep in mind, this will also change the catalog listing to work the same way as tags.

This seems related to this discussion: opencontainers/distribution-spec#132

Okay, so what I think should happen is the following.

• n=0 will return a empty list, and will not include a Link, as there are no more entries (depends on how you look at it. But if I want 0 and I want more, I will still get 0).
• last will be used to find the entries lexically after it, and the index does not need to exists.
  • this could bring some weird bugs for some people regarding encoding. but it does make sense to be able to search like this.
• last can stand on its own without n, as to use the default n value from the server (which it may impose).

How does this sound @jdolitsky and @dmcgowan ?

Makes sense to me, but we should probably clarify in the spec first. I'm ok making that update in this project with expectation that we may need to make slight changes if we come to a different agreement for the spec

@dmcgowan so this is good to go, if I make the changes and then we can update the spec in accordance?

yeah, let's go ahead and make those changes and get this merged in. We can tweak the conformance tests and spec after, if we need to come back and sync here we can before the release

@dmcgowan thougt I had pinged you here, I guess not. It should be good to go! Travis is giving some weird errors, that doesn't seem to be a problem with the code. Should be ready to merge!

According to the documentation for sort.SearchStrings, x is not present (it could be len(a)). This could lead to a panic here. This should probably be covered in the unit tests and boundary tested.

I just seem to remember having fixed that error before.
To do unittests, that would require I know a lot more regarding how unit tests are done in the handlers.

I propose making this change (fixing this), so it will not panic and then we can either merge and someone can make a PR for unit tests, or there can be made a PR based on this.

But considering this has been in the making since 2016, I am not having high hopes for anyone picking up the unittests. And not sure when I will get the time for it, sadly

@eyJhb maybe at least to ensure no panic we can add something like

if lastEntry := q.Get("last"); lastEntry != "" {
	lastEntryIndex := sort.SearchStrings(tags, lastEntry) + 1
	if lastEntryIndex < len(tags) {
		tags = tags[lastEntryIndex:]
	}
}

?

@jdolitsky ahh damn it, that is a lot prettier than what I just pushed... Lets totally go with that! Let me just rebase again :)

EDIT: we can't do that actually, as we need to set it to a empty list if it has no matches. That is at least what I would think if a user specifies something that is not in the tags (return nothing). But we can do this

What about the newest change?

	if lastEntry := q.Get("last"); lastEntry != "" {
		lastEntryIndex := sort.SearchStrings(tags, lastEntry)

		// as`sort.SearchStrings` can return len(tags), if the
		// specified `lastEntry` is not found, we need to
		// ensure it does not panic when slicing.
		if lastEntryIndex < len(tags) {
			tags = []string{}
		} else {
			tags = tags[lastEntryIndex+1:]
		}
	}

Yes, noticed that too. I agree. This lgtm

I did however like yours a lot, but we end up having somewhat the same :) Perfect! Lets see if the others agree.
@dmcgowan will you resolve if you think this solution is acceptable?

Ping @dmcgowan :)

Ping again, hope it is okay @dmcgowan :)

@thaJeztah anything we can do to get this merged?

IMO, it's better to put the sorting in the tagHandler, just let's the tags cosumer to judge.

if the last doesn't exist, should not be a bad request?

We can do partial searches, using the lastEntryIndex, so it doesn't need to be a exact match, and therefore is not really a error as far as I understand :)