Create typed IRI concept for Publication Channels#2437
Conversation
Test Results 286 files + 2 286 suites +2 24m 7s ⏱️ +12s Results for commit e2e57eb. ± Comparison against base commit 5263c03. This pull request removes 304 and adds 195 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| public SerialPublicationId(UuidYearPair validate) { | ||
| this(validate.uuid(), validate.year()); | ||
| } |
There was a problem hiding this comment.
Input variable name is wrong.
| static PublicationChannelId from(URI uri) { | ||
| if (nonNull(uri) && uri.toString().contains(ChannelType.PUBLISHER.getType())) { | ||
| return PublisherId.from(uri); | ||
| } else if (nonNull(uri) && uri.toString().contains(ChannelType.SERIAL_PUBLICATION.getType())) { | ||
| return SerialPublicationId.from(uri); | ||
| } else { | ||
| throw new IllegalArgumentException("Encountered URI that is not a valid publication channel id"); | ||
| } | ||
| } |
There was a problem hiding this comment.
If we want to be very strict, should we validate that ChannelType.PUBLISHER.getType() and ChannelType.SERIAL_PUBLICATION.getType() are in right places? It seems that we do it downstream :)
There was a problem hiding this comment.
Happens downstream, this is simply to differentiate the two so they end up being processed correctly.
| String value(); | ||
|
|
||
| static String value(ChannelType type, UUID identifier, Year year) { | ||
| return "/%s/%s/%s/%s".formatted(PATH_ELEMENT_ONE, type.getType(), identifier, year); |
There was a problem hiding this comment.
You have PATH_TEMPLATE to use here instead of string
| } | ||
|
|
||
| default URI uri(String host) { | ||
| if (isNull(host) || host.isBlank()) { |
There was a problem hiding this comment.
StringUtils.isBlank() combines it in single method
There was a problem hiding this comment.
Not sure that I want the entirety of commons here yet.
| import static java.util.Objects.isNull; | ||
| import static java.util.Objects.nonNull; | ||
|
|
||
| public interface PublicationChannelId { |
There was a problem hiding this comment.
90% of this interface is common validation rules for PublicationChannelId, in addition to 4 public getters(). What about moving all the validation to PublicationChannelIdValidator? Separation of concerns :)
There was a problem hiding this comment.
Yep, this is a good idea. I want to see if this idea works for other types and the validator concept will be more important then since a lot of the IRIs we create are structured in similar ways.
| } | ||
|
|
||
| public static SerialPublicationId from(URI uri) { | ||
| return new SerialPublicationId(PublicationChannelId.validate(uri, CHANNEL_TYPE)); |
There was a problem hiding this comment.
Validate method implies void or boolean, but here we return an object.
What about do validation and then extract identifier and year by methods in common interface?
public static SerialPublicationId from(URI uri) {
PublicationChannelId.validate(uri, CHANNEL_TYPE);
var identifier = PublicationChannelId.extractIdentifier(uri);
var year = PublicationChannelId.extractYear(uri);
return new SerialPublicationId(identifier, year);
}
Thank we may no need wrapper object UuidYearPair
There was a problem hiding this comment.
Yep, this was a quick fix for an earlier issue, but I think it is probably important that we know the pair is a single thing (the identifier)
| import java.time.Year; | ||
| import java.util.UUID; | ||
|
|
||
| public record SerialPublicationId(UUID identifier, Year year) implements PublicationChannelId { |
There was a problem hiding this comment.
Record has default constructor which allows us to instantiate invalid PublicationChannelId, for example:
Maybe it should be a class with private constructor and factory method which enforces user to validate object before instantiation.
@Test
void possibleToInstantiateInvalidPublicationChannelId() {
assertDoesNotThrow(() -> new SerialPublicationId(null, Year.parse("1999")));
}
There was a problem hiding this comment.
I think what the interface is is a factory…I will refactory it :D
Motivation
The major motivation for this is to simplify operations related to date checking in what is essentially a composite string.
/{type}/{identifier}/{year}Secondly, the data is now validated
Thirdly, we are interested in the data abstract from the environment
What is done
PublicationChannelId, which is now a concept in the model. This thing is not a "journal", nor is it a "publisher", it is simply a representation of the data in@id. It provides accessors to the elements that form a valid publication channel id and fails with a relevant error message if we encounter something that is not as it should be.fromUri(URI uri)creates thePublicationChannelIdfrom a URIuri(String host)creates the URI from thePublicationChannelIdvalue()provides the concatenated string for JSON output.Usage
Initially, we will use
PublicationChannelId::fromUriandPublicationChannelId::urito migrate the platform away from URI.Later, we will use
PublicationChannelId::valueto remove the URI from persisted data.This latter statement sounds weird, how will that work?
Consider the following, noting the
@baseand lack of host in the channel@id:{ "@context": { "@vocab": "https://schema.org", "@base": "https://api.nva.unit.no" }, "channel": { "@id": "/publication-channels-v2/serial-publication/360B8D2C-736F-450A-8D34-9596BFE28CB4/2025", "@type": "Journal", "owner": "Casey", "label": "Something" } }This serialises to N-Triples as:
Since our base is relative to the environment we are in, and we already know this from the environment variables, this change is a simple way of making the data portable between the environments (noting that we do not persist the context in the database). Where we do persist the context, we can check one field in the
@contextobject rather than any field containing a URI that may or may not match a predefined expectation.Downside
@contextbecomes environment specific