feature/verify many

[mprorock]

a way of handling #88

it became clear to me when working this up and testing some things that there is an additional issue in the way that resources / controllers are named as there is a clearer way to structure the end points in compliance with the restful api guidelines. i will open a separate issue on this for discussion.

as is this PR does not break any existing implementations against credentials/verify and simply adds the ability to verifyMany by passing in an array of objects, and returning back an array of results that matches the array that was passed into the new method

[msporny]

-1 to batch processing APIs. It needs to be demonstrated that the API, as it stands today, is not capable of achieving the desired processing rates.

[OR13]

@mprorock can you implement the proposal made here: #88 (comment)

[mprorock]

@OR13 that can be done, the issue is that the swagger UI does not handle oneOf very well

see swagger (note the "no parameters"):

vs redoc (handles properly):

This kindof thing can also be done with nullable: true on parameters that can be set to explicitly to null but then the docs do not clearly mark the parameter as optional or nullable and it is easier for developers to make mistakes. pr #99 introduces redoc as an option and makes the oneOf option doable, which is much more clear to developers coming into a spec than a nullable that has a variety of UI presentation handlings

[OR13]

@mprorock oneOf is valid JSON Schema.... if the built in render fails, but redoc succeeds and the JSON Schema is valid.... seems like redoc is a better long term choice... the important part is that the API contract is expressed clearly.... If holder is present... proof must be present.

[mprorock]

@OR13 removed verifyMany from the PR based on proposal in #88 and also in reference to discussion on #97

[OR13]

This change set appears to solve the user story:

"As a developer I want to verify multiple credentials at once"

without preventing the user story:

"As a developer I want to map an array of credentials too http requests so I can await multiple promises to verify multiple credentials at once."

[msporny]

@OR13 wrote:

This change set appears to solve the user story

It does it by adding unnecessary complexity... it hasn't been shown why the current APIs are not capable of addressing the use case.

-1 to this feature, more documentation here: #100

[peacekeeper]

I tend to agree with @mprorock that verifying a presentation with multiple credentials isn't really "batch processing", so I think this use case should be supported by the API. But I would comment that:

1. Looking at the current PR, it sounds a bit weird if the endpoint is called "/presentations/verify", but it would now allow a client to pass in something that's considered a "presentation" but not a "verifiable presentation". How do you verify a non-verifiable presentation? (I know what you mean, just saying that this could confuse people; maybe there's a better way of modeling the same thing).
2. Perhaps add a comment which says that implementations may choose to only accept presentations with a single credential (e.g. to prevent Denial-of-Service attacks or other problems mentioned in Verifiable Credential Batch HTTP APIs are an Anti-pattern #100).

[mprorock]

1. Looking at the current PR, it sounds a bit weird if the endpoint is called "/presentations/verify", but it would now allow a client to pass in something that's considered a "presentation" but not a "verifiable presentation". How do you verify a non-verifiable presentation? (I know what you mean, just saying that this could confuse people; maybe there's a better way of modeling the same thing).
   yeah, this is one of the issues of multiple layers of "verify" in this case the api is a controller that acts on "presentations". in some cases those are presentations with proof, e.g. "verifiable presentation" and in other cases, just a "presentation".

open to suggestions on better language here, perhaps "validate", however the issue then, is that existing implementations of the API would have to be updated.

2. Perhaps add a comment which says that implementations may choose to only accept presentations with a single credential (e.g. to prevent Denial-of-Service attacks or other problems mentioned in Verifiable Credential Batch HTTP APIs are an Anti-pattern #100).

this is very good idea. I will add a comment to the descriptor

[msporny]

@peacekeeper can you re-review this?

I'm going to hold for 24 hours for a review by @peacekeeper ... if we don't get one in that time frame, let's merge.

[msporny]

@mprorock wrote:

@msporny totally fine either way - if you would prefer a rebase happy to do so.

I always prefer to rebase -- it keeps a chronological record of what we tried/changed/did/etc. so we can refer to it days, weeks, or years from now. It also protects all of us from an Intellectual Property perspective. If someone comes in with a patent years from now, we have a record of when a feature was clearly in the public domain (before the patent was filed). You can always do an interactive rebase and squash away the things you don't feel need to be saved. I'd rather the PR authors do this than the Editors (who don't have the full picture in their head when rebasing -- and we typically don't have access to your forked repo). Food for thought.

[mprorock]

I always prefer to rebase

Makes total sense - will push shortly

[peacekeeper]

Sorry for the delay, looks good now!

[peacekeeper]

Still waiting to rebase, or do you want us to squash and merge? @mprorock

[mprorock]

@peacekeeper i did pull and merge upstream, and then rebase the branch - so in theory all history should be persevered. please do verify though

[peacekeeper]

@mprorock hmm it still says "This branch cannot be rebased due to conflicts"..

[mprorock]

@peacekeeper - mind if i close this, branch fresh since so many changes have gone through, and reopen a new pull?

[TallTed]

@peacekeeper @mprorock

There seems to be something weird happening on this PR.

@peacekeeper is told "This branch cannot be rebased due to conflicts", but I see "This branch has no conflicts with the base branch".

If @msporny doesn't get a conflict warning, I think merging this PR is the best option, because of the volume of changes and complexity of change history (31 commits from several authors). I'm not sure how best to handle that if the PR needs recreation.

[mprorock]

@TallTed I see a no conflicts message as well - but want to make sure that nothing got rolled back. we have had quite a few merges into master since this pr originally went up

[peacekeeper]

@mprorock and @TallTed it only says there are conflicts if I try "Rebase and merge". For "Squash and merge" there are no conflicts.. So I could just do that, but as @msporny says this doesn't really preserve the history of the individual commits (I think this would be equal to your "close and branch fresh" suggestion).

I guess if we can't figure out how to resolve the "rebase and merge" conflicts, we just go ahead and "squash and merge".

[mprorock]

@peacekeeper does it show where the rebase conflicts are?

[mprorock]

when I run a compare it shows one file changed, with only correct changes and no conflicts.... @msporny or @OR13 any insights as to why this is showing a conflict for @peacekeeper ?

[msporny]

when I run a compare it shows one file changed, with only correct changes and no conflicts.... @msporny or @OR13 any insights as to why this is showing a conflict for @peacekeeper ?

Oof... this PR now has 29 files associated with it -- something went very wrong at d95958d.

I'm seeing the same rebase conflicts that @peacekeeper is seeing... because there are rebase conflicts. :P

The PR is now destroyed beyond repair due to the non-rebased merge d95958d (never do a multi-head merge into a branch you're trying to get a clean rebase on). :) -- git can be unforgiving. I could fix it if I had access to measur-io's repo and/or enough time, but I don't.

If @mprorock doesn't want to do the surgery to save this branch (at this point a hard reset to b77b7d7 on his branch, followed with a rebase on top of master (with appropriate rebase conflict fixes along the way), followed with an interactive rebase to merge commits to clean up commit history), I suggest he starts over with a clean PR.

[mprorock]

Let's just squash it.

[msporny]

Let's just squash it.

By Odin's Beard. Done.