Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return empty list if there is no siriUrls in situations/infoLinks #6232

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Return empty list if there is no siriUrls in situations/infoLinks #6232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d92a7fe
fix: Return empty list if there is no siriUrls in situations[]/infoLi…
t2gran Nov 6, 2024
f492446
Ignore situation alerts without url or siri uris
t2gran Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...ation/src/main/java/org/opentripplanner/apis/transmodel/model/framework/InfoLinkType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public static GraphQLObjectType create() {
.description("URI")
.dataFetcher(environment -> {
AlertUrl source = environment.getSource();
return source.uri;
return source.uri();
})
.build()
)
Expand All @@ -32,7 +32,7 @@ public static GraphQLObjectType create() {
.description("Label")
.dataFetcher(environment -> {
AlertUrl source = environment.getSource();
return source.label;
return source.label();
})
.build()
)
Expand Down
2 changes: 1 addition & 1 deletion ...c/main/java/org/opentripplanner/apis/transmodel/model/siri/sx/PtSituationElementType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ public static GraphQLObjectType create(
if (!siriUrls.isEmpty()) {
return siriUrls;
}
return null;
return emptyList();
})
.build()
)
Expand Down
22 changes: 17 additions & 5 deletions application/src/main/java/org/opentripplanner/framework/i18n/I18NString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.opentripplanner.framework.i18n;

import java.util.Locale;
import javax.annotation.Nullable;

/**
* This interface is used when providing translations on server side. Sources: OSM tags with
Expand All @@ -9,9 +10,20 @@
* @author mabu
*/
public interface I18NString {
/** true if the given value is not {@code null} or has at least one none white-space character. */
public static boolean hasValue(I18NString value) {
return value != null && !value.toString().isBlank();
/**
* Return {@code true} if the given value is not {@code null} or has at least one none
* white-space character.
*/
static boolean hasValue(@Nullable I18NString value) {
return !hasNoValue(value);
}

/**
* Return {@code true} if the given value has at least one none white-space character.
* Return {@code false} if the value is {@code null} or blank.
*/
static boolean hasNoValue(@Nullable I18NString value) {
return value == null || value.toString().isBlank();
}

/**
Expand All @@ -26,8 +38,8 @@ public static boolean hasValue(I18NString value) {
*/
String toString(Locale locale);

static I18NString assertHasValue(I18NString value) {
if (value == null || value.toString().isBlank()) {
static I18NString assertHasValue(@Nullable I18NString value) {
if (hasNoValue(value)) {
throw new IllegalArgumentException(
"Value can not be null, empty or just whitespace: " +
(value == null ? "null" : "'" + value + "'")
Expand Down
9 changes: 6 additions & 3 deletions application/src/main/java/org/opentripplanner/routing/alertpatch/AlertUrl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package org.opentripplanner.routing.alertpatch;

public class AlertUrl {
import java.util.Objects;
import javax.annotation.Nullable;

public String uri;
public String label;
public record AlertUrl(String uri, @Nullable String label) {
public AlertUrl {
Objects.requireNonNull(uri);
}
}
47 changes: 29 additions & 18 deletions application/src/main/java/org/opentripplanner/updater/siri/SiriAlertsUpdateHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,9 @@ private TransitAlert mapSituationToAlert(
TransitAlertBuilder alert = createAlertWithTexts(situation);

if (
(alert.headerText() == null || alert.headerText().toString().isEmpty()) &&
(alert.descriptionText() == null || alert.descriptionText().toString().isEmpty()) &&
(alert.detailText() == null || alert.detailText().toString().isEmpty())
I18NString.hasNoValue(alert.headerText()) &&
I18NString.hasNoValue(alert.descriptionText()) &&
I18NString.hasNoValue(alert.detailText())
Comment on lines +138 to +140
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check not be part of TransitAlertBuilder?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should and I first put it in the builder - but the builder should not have the validation logic, the main class should. The problem is that if we validate when we construct the new entity, I would have to change the SiriAlertsUpdateHandler more than I like for a simple bug fix.

) {
LOG.debug(
"Empty Alert - ignoring situationNumber: {}",
Expand Down Expand Up @@ -221,18 +221,18 @@ private long getEpochSecond(ZonedDateTime startTime) {
private TransitAlertBuilder createAlertWithTexts(PtSituationElement situation) {
return TransitAlert
.of(new FeedScopedId(feedId, situation.getSituationNumber().getValue()))
.withDescriptionText(getTranslatedString(situation.getDescriptions()))
.withDetailText(getTranslatedString(situation.getDetails()))
.withAdviceText(getTranslatedString(situation.getAdvices()))
.withHeaderText(getTranslatedString(situation.getSummaries()))
.withUrl(getInfoLinkAsString(situation.getInfoLinks()))
.addSiriUrls(getInfoLinks(situation.getInfoLinks()));
.withDescriptionText(mapTranslatedString(situation.getDescriptions()))
.withDetailText(mapTranslatedString(situation.getDetails()))
.withAdviceText(mapTranslatedString(situation.getAdvices()))
.withHeaderText(mapTranslatedString(situation.getSummaries()))
.withUrl(mapInfoLinkToI18NString(situation.getInfoLinks()))
.addSiriUrls(mapInfoLinks(situation));
}

/*
* Returns first InfoLink-uri as a String
*/
private I18NString getInfoLinkAsString(PtSituationElement.InfoLinks infoLinks) {
private I18NString mapInfoLinkToI18NString(PtSituationElement.InfoLinks infoLinks) {
if (infoLinks != null) {
if (isNotEmpty(infoLinks.getInfoLinks())) {
InfoLinkStructure infoLinkStructure = infoLinks.getInfoLinks().get(0);
Expand All @@ -247,21 +247,32 @@ private I18NString getInfoLinkAsString(PtSituationElement.InfoLinks infoLinks) {
/*
* Returns all InfoLinks
*/
private List<AlertUrl> getInfoLinks(PtSituationElement.InfoLinks infoLinks) {
private List<AlertUrl> mapInfoLinks(PtSituationElement situation) {
PtSituationElement.InfoLinks infoLinks = situation.getInfoLinks();
List<AlertUrl> alertUrls = new ArrayList<>();
if (infoLinks != null) {
if (isNotEmpty(infoLinks.getInfoLinks())) {
for (InfoLinkStructure infoLink : infoLinks.getInfoLinks()) {
AlertUrl alertUrl = new AlertUrl();

String label = null;
List<NaturalLanguageStringStructure> labels = infoLink.getLabels();
if (labels != null && !labels.isEmpty()) {
NaturalLanguageStringStructure label = labels.get(0);
alertUrl.label = label.getValue();
NaturalLanguageStringStructure lbl = labels.get(0);
label = lbl.getValue();
}

alertUrl.uri = infoLink.getUri();
alertUrls.add(alertUrl);
var uri = infoLink.getUri();
if (uri != null) {
alertUrls.add(new AlertUrl(uri, label));
} else {
if (LOG.isDebugEnabled()) {
LOG.debug(
"URI missing in info-link - ignoring info-link in situation: {}",
situation.getSituationNumber() != null
? situation.getSituationNumber().getValue()
: null
);
}
}
}
}
}
Expand All @@ -281,7 +292,7 @@ private boolean isNotEmpty(List<?> list) {
*
* @return A TranslatedString containing the same information as the input
*/
private I18NString getTranslatedString(List<DefaultedTextStructure> input) {
private I18NString mapTranslatedString(List<DefaultedTextStructure> input) {
Map<String, String> translations = new HashMap<>();
if (input != null && input.size() > 0) {
for (DefaultedTextStructure textStructure : input) {
Expand Down
32 changes: 32 additions & 0 deletions application/src/test/java/org/opentripplanner/framework/i18n/I18NStringTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package org.opentripplanner.framework.i18n;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

import org.junit.jupiter.api.Test;

class I18NStringTest {

private final I18NString noValue = I18NString.of(" \t\n\r\f");
private final I18NString hasValue = I18NString.of("A value");

@Test
void hasValue() {
assertTrue(I18NString.hasValue(hasValue));
assertFalse(I18NString.hasValue(noValue));
}

@Test
void hasNoValue() {
assertFalse(I18NString.hasNoValue(hasValue));
assertTrue(I18NString.hasNoValue(noValue));
}

@Test
void assertHasValue() {
var ex = assertThrows(IllegalArgumentException.class, () -> I18NString.assertHasValue(noValue));
assertEquals("Value can not be null, empty or just whitespace: ' \t\n\r\f'", ex.getMessage());
}
}
4 changes: 2 additions & 2 deletions application/src/test/java/org/opentripplanner/updater/siri/SiriAlertsUpdateHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,8 @@ public void testSiriSxUpdateForStop() {

final List<AlertUrl> alertUrlList = transitAlert.siriUrls();
AlertUrl alertUrl = alertUrlList.get(0);
assertEquals(infoLinkUri, alertUrl.uri);
assertEquals(infoLinkLabel, alertUrl.label);
assertEquals(infoLinkUri, alertUrl.uri());
assertEquals(infoLinkLabel, alertUrl.label());
}

@Test
Expand Down
Loading