providers: provide DataCite-like DOI locally

[fenekku]

• Generate a random, configurable length, base32, URI-friendly, hyphen-separated,
  optionally checksummed DOI
• Closes pid: generate random alphanumeric PIDs #124

It would probably be good to have an @inveniosoftware/architects reviewing this too because it generates a PID (DOI) which is a core element of Invenio and it does so in a non-trivial way (I think).

[ntarocco]

Great work!!! 💯 🚀

May I ask to slightly change the code to be even more reusable?
I would like to use base32 for normal recids, not only DOIs.
123456789 -> ABCD-EDFG

can we "plug it in" somehow in the recordid.py? Optionally, to avoid to break backward-compatibility.

[fenekku]

Had to add min_length to ensure encoded length, without this tests were failing on Travis.

[ppanero]

LGTM, tho in the separate module as discussed IRL.

[ntarocco]

LGTM! Good job! 👍 🚀 💯

Question, maybe for @lnielsen too: do we want in the long run in Invenio to have recids with this new base32 format? If yes, shall we create another RecordId provider that generate such IDs?
In our case, we will have to re-create that provider both in ILS and future CDS... I am wondering if it makes sense to put it here so that it is available to everyone...

[lnielsen]

There's an overall issue here, that I'm sorry I haven't detected before. We need a PIDProvider that's able to generate internal identifiers (base32) - a bit similar to RecordIdProvider. The DataCite provider should generate the DOI by taking the internal identifier and use it as suffix.

[ppanero]

🚀 Great job!

A few minors, check in depth the test comments, there might be an inconsistency in the "default" length/split.

Approved: Assuming passing tests :)

[fenekku]

Thanks for the review @ppanero . Changes made. Just needs an architect to also approve.

[ntarocco]

LGTM! Fantastic work! Thanks for this! :)

A couple of comments:

• I guess the V2 naming/pattern and deprecation of current RecordIdProvider was agreed with @lnielsen and/or @ppanero?
• I understand that the potential collision of a random generated base32 id is probably very minimal, but I am wondering if we should check if there is a collision, e.g. (pseudocode):

while RecordIdProviderV2.get(pid_value):
    pid_value = cls.generate_id(options)

kwargs['pid_value'] = pid_value

I fear we might have the db raising exception if the generated id is already in the table...

[fenekku]

Fixed the bash header.

1- Naming is always going to be hard. I think it's fine.
2- Still not sure if I understand the concerns about this:

I fear we might have the db raising exception if the generated id is already in the table...

I don't fear it... I hope for it? We do want an exception if the id is already in the table and we
will get one according to: https://github.com/inveniosoftware/invenio-pidstore/blob/master/invenio_pidstore/models.py#L163
We are not trying to recover from it, because user can just retry. If we do try to recover ourselves, it would lead to a rabbit hole of other extremely rare edge cases it seems to me (I am open to ideas to do recovery sanely if people have them). Checking for existence beforehand would be a consistently big performance hit for an incredibly rare 'payoff'. I'd rather move on given the time spent on the PR...

[lnielsen]

Regarding retrying on collisions: I would leave it as is, which is consistent with how we deal with uuid's for e.g. records.

[fenekku]

@lnielsen Build is passing and it has been approved: this can be merged.

[fenekku]

@lnielsen : to be more explcit: neither Pablo or I can merge this, so if an architect could merge it, it would unblock us :)