add fake PID provider for devs #5327 #5399
Conversation
There was a problem hiding this comment.
Code itself looks fine by reading. Wasn't able to test (functionality, display of FAKE PIDs) due to gremlins apparently unrelated to this PR.
Open question about if it makes sense to update phoenix configs, docker-aio, vagrant (potentially other docker stuff too) to use this config. Will retry to see if the gremlins go away before worrying about that.
| @@ -154,6 +155,16 @@ protected void registerExternalIdentifier(Dataset theDataset, CommandContext ctx | |||
| if (!theDataset.isIdentifierRegistered()) { | |||
| GlobalIdServiceBean globalIdServiceBean = GlobalIdServiceBean.getBean(theDataset.getProtocol(), ctxt); | |||
| if ( globalIdServiceBean != null ) { | |||
| if (globalIdServiceBean instanceof FakePidProviderServiceBean) { | |||
There was a problem hiding this comment.
IMHO doubling the code from below for the sake of the fake provider is a suboptimal approach.
Why does alreadyExists() always return true? If it would return false, the existing code from below could be used instead.
|
|
||
| @Override | ||
| public boolean alreadyExists(DvObject dvo) throws Exception { | ||
| return true; |
There was a problem hiding this comment.
See my comment above.
This could be extended, too: when the fake provider would add a number to a simple database field, that could be used for constantly growing index number usable as a "real" faked PID. Other things like UUIDs or similar could be used, which are garantied to be doublication free. A third alternative would be to lookup the highest number in the database, increment and use. That is slow, but for demo / dev purposes this could be alright.
|
|
||
| @Override | ||
| public boolean registerWhenPublished() { | ||
| return false; |
There was a problem hiding this comment.
Maybe this should be a setting instead? It might be usefull during demos of the code.
| public List<String> getProviderInformation() { | ||
| ArrayList<String> providerInfo = new ArrayList<>(); | ||
| String providerName = "FAKE"; | ||
| String providerLink = "http://dataverse.org"; |
There was a problem hiding this comment.
Could this be replaced with the dataverse FQDN/URL? That way we could generate fake, but working PID URLs pointing to the instance, couldn't we?
|
|
||
| @Override | ||
| public String createIdentifier(DvObject dvo) throws Throwable { | ||
| return "fakeIdentifier"; |
There was a problem hiding this comment.
See above about providing fake but usable PIDs.
|
|
||
| @Override | ||
| public Map<String, String> lookupMetadataFromIdentifier(String protocol, String authority, String identifier) { | ||
| Map<String, String> map = new HashMap<>(); |
There was a problem hiding this comment.
Couldn't this at least return some stuff from the DvObject if the identifier is fake but usable?
|
At standup just now I told @landreev and others that I'd leave a comment here to note that @poikilotherm and I have talked about his comments in IRC and he and I are on the same page with this pull request being an incremental improvement. In the future, if this fake PID provider proves useful to developers, we can hack on the code some more and make it better. @sekmiller pointed out that the fake PID providers doesn't support file DOIs which is a good catch but I don't believe it needs to. The REST Assured test suite passes without it. If we ever need to add support for file DOIs, Steve said we need to write some code in the @matthew-a-dunlap said he'd do some code review (thanks!) and I'd like to point out that I added a "connect to" the new issue I just created about phoenix tests failing: #5409. |
connects to #5327
connects to #5409