GG-380 Add test for multitable types missing table. by Erkelinux · Pull Request #346 · sikt-no/graphitron

Erkelinux · 2026-02-12T11:29:21Z

Also made the check throw all errors for one union/interface at once, rather than one by one.

andreahn · 2026-02-16T08:29:18Z

Prøvde å få feilen i graphitron-example ved å fjerne tabell fra StaffWithEmail, men da får jeg heller en valideringsfeil på referanse (og kun det):

Error on field "addressAsSubquery" in type "StaffWithEmail": No foreign key found between tables "RENTAL" and "ADDRESS". Please specify path with the @reference directive.

Dette er antageligvis på grunn av at Payment har et felt som peker mot StaffWithEmail, og derfor blir forrige tabell RENTAL. Det blir med andre ord en følgefeil av at StaffWithEmail mangler tabell, men vi får aldri den egentlige feilen (at typen mangler tabell) siden den blir kastet i kodegenereringen.

Jeg syns vi burde flyttet dette til ProcessedDefinitionsValidator sånn at vi får feilmeldingen uavhengig av om noe annet feiler i valideringssteget

…s and tests.

Erkelinux · 2026-03-03T15:35:35Z

Prøvde å få feilen i graphitron-example ved å fjerne tabell fra StaffWithEmail, men da får jeg heller en valideringsfeil på referanse (og kun det):

Error on field "addressAsSubquery" in type "StaffWithEmail": No foreign key found between tables "RENTAL" and "ADDRESS". Please specify path with the @reference directive.

Dette er antageligvis på grunn av at Payment har et felt som peker mot StaffWithEmail, og derfor blir forrige tabell RENTAL. Det blir med andre ord en følgefeil av at StaffWithEmail mangler tabell, men vi får aldri den egentlige feilen (at typen mangler tabell) siden den blir kastet i kodegenereringen.

Jeg syns vi burde flyttet dette til ProcessedDefinitionsValidator sånn at vi får feilmeldingen uavhengig av om noe annet feiler i valideringssteget

Prøvde å rydde opp i dette nå, og tilpasset slik at dette bare blir sjekket det ene stedet siden jeg fant at dette ble allerede validert andre steder (men det ser ikke ut til å plukke opp alt?)

andreahn · 2026-03-05T08:27:00Z

...gen-parent/graphitron-java-codegen/src/main/java/no/sikt/graphql/schema/ProcessedSchema.java

     * @return Get the implementations for an interface given its name
     */
-    public Set<ObjectDefinition> getImplementationsForInterface(String interfaceName) {
+    public List<ObjectDefinition> getImplementationsForInterface(String interfaceName) {


Hvorfor har du endra fra Set til List?

Set var ikke brukt til noe konkret og rekkefølge var påkrevd flere steder uannsett. Her trengte jeg vel rekkefølge på noe og innså at Set ble uannsett konvertert til ordnede strukturer og tenkte at det er like greit at den er liste fra start

andreahn · 2026-03-05T08:29:06Z

...gen-parent/graphitron-java-codegen/src/main/java/no/sikt/graphql/schema/ProcessedSchema.java

@@ -399,19 +398,11 @@ public Set<ObjectDefinition> getTypesFromInterfaceOrUnion(String name) {
        return null;
    }


Suggested change

public Optional<List<ObjectDefinition>> getTypesFromInterfaceOrUnion(String name) {

if (isUnion(name)) {

return Optional.of(getUnionSubTypes(isConnectionObject(name) ? getConnectionObject(name).getNodeType() : name));

}

if (isInterface(name)) {

return Optional.of(getImplementationsForInterface(isConnectionObject(name) ? getConnectionObject(name).getNodeType() : name));

}

return Optional.empty();

}

hvis du først skal begynne å fikse her 🤷🏻

Tok det med, men opplever at det bare ble litt ekstra støy andre steder

andreahn · 2026-03-05T08:32:16Z

...arent/graphitron-java-codegen/src/main/java/no/sikt/graphitron/validation/ErrorMessages.java

+public enum ErrorMessages {
+    MISSING_FIELD("Input type %s referencing table %s does not map all fields required by the database. Missing required fields: %s"),
+    MISSING_NON_NULLABLE("Input type %s referencing table %s does not map all fields required by the database as non-nullable. Nullable required fields: %s"),
+    MISSING_TABLE_ON_MULTITABLE("Type(s) '%s' are used in a query '%s.%s' returning multitable interface or union '%s', but do not have tables set. This is not supported.");
+
+    private final String msg;
+
+    ErrorMessages(String msg) {
+        this.msg = msg;
+    }
+
+    public String getMsg() {
+        return msg;
+    }


Jeg er litt usikker på om jeg er så fan av dette. Det gjør det mye vanskeligere å lese testene vi har på validering

Gjør om testene tilbake

andreahn · 2026-03-05T08:35:42Z

...-java-codegen/src/main/java/no/sikt/graphitron/validation/ProcessedDefinitionsValidator.java

+                .filter(it -> !it.getTypeName().equals(NODE_TYPE.getName()))
+                .forEach((field) -> {
+                    var multitableName = field.getTypeName();
+                    if (multitableName.equalsIgnoreCase(NODE_TYPE.getName()) || multitableName.equalsIgnoreCase(ERROR_TYPE.getName())) {


Suggested change

if (multitableName.equalsIgnoreCase(NODE_TYPE.getName()) || multitableName.equalsIgnoreCase(ERROR_TYPE.getName())) {

if (multitableName.equalsIgnoreCase(ERROR_TYPE.getName())) {

Node blir vel filtrert ut over

andreahn · 2026-03-05T08:38:01Z

...-java-codegen/src/main/java/no/sikt/graphitron/validation/ProcessedDefinitionsValidator.java

+                                            if (implementation.getTable() != null && !tableHasPrimaryKey(implementation.getTable().getName())) {
                                                addErrorMessage("Interface '%s' is returned in field '%s', but implementing type '%s' " +
                                                        "has table '%s' which does not have a primary key. This is not supported.", name, field.getName(), implementation.getName(), implementation.getTable().getName());


Hvorfor endra du på det her? Det er to forskjellige feil

andreahn · 2026-03-05T08:40:31Z

...-java-codegen/src/main/java/no/sikt/graphitron/validation/ProcessedDefinitionsValidator.java

+                            .map(AbstractObjectDefinition::getName)
+                            .collect(Collectors.joining("', '"));
+                    if (!typesMissingTable.isEmpty()) {
+                        addErrorMessage(MISSING_TABLE_ON_MULTITABLE.getMsg(), typesMissingTable, field.getContainerTypeName(), field.getName(), field.getTypeName());


Suggested change

addErrorMessage(MISSING_TABLE_ON_MULTITABLE.getMsg(), typesMissingTable, field.getContainerTypeName(), field.getName(), field.getTypeName());

addErrorMessage(MISSING_TABLE_ON_MULTITABLE.getMsg(), typesMissingTable, field.formatPath(), field.getTypeName());

andreahn · 2026-03-05T08:41:18Z

...arent/graphitron-java-codegen/src/main/java/no/sikt/graphitron/validation/ErrorMessages.java

+public enum ErrorMessages {
+    MISSING_FIELD("Input type %s referencing table %s does not map all fields required by the database. Missing required fields: %s"),
+    MISSING_NON_NULLABLE("Input type %s referencing table %s does not map all fields required by the database as non-nullable. Nullable required fields: %s"),
+    MISSING_TABLE_ON_MULTITABLE("Type(s) '%s' are used in a query '%s.%s' returning multitable interface or union '%s', but do not have tables set. This is not supported.");


Suggested change

MISSING_TABLE_ON_MULTITABLE("Type(s) '%s' are used in a query '%s.%s' returning multitable interface or union '%s', but do not have tables set. This is not supported.");

MISSING_TABLE_ON_MULTITABLE("Type(s) '%s' are used in a query '%s' returning multitable interface or union '%s', but do not have tables set. This is not supported.");

andreahn · 2026-03-05T08:42:34Z

...rent/graphitron-java-codegen/src/test/java/no/sikt/graphitron/validation/MultitableTest.java

+    void implementationWithoutTable() {
+        assertErrorsContain(
+                () -> getProcessedSchema("implementationWithoutTable"),
+                String.format(MISSING_TABLE_ON_MULTITABLE.getMsg(), "Customer", "Query", "someInterface", "SomeInterface")


Jeg tror jeg syns dette blir litt vel uleselig 🤔

Erkelinux requested review from andreahn and folkef6 February 12, 2026 11:29

Erkelinux mentioned this pull request Feb 12, 2026

GG-355 Fix entity data fetcher compatibility. #344

Merged

Erkelinux added 2 commits March 3, 2026 14:15

GG-380 Add test for multitable types missing table.

9d74e59

GG-380 Adjusted validation to run earlier and removed duplicate check…

df5f70e

…s and tests.

Erkelinux force-pushed the GG-380-add-tests branch from da0d7f7 to df5f70e Compare March 3, 2026 15:33

andreahn reviewed Mar 5, 2026

View reviewed changes

		@@ -399,19 +398,11 @@ public Set<ObjectDefinition> getTypesFromInterfaceOrUnion(String name) {
		return null;
		}

+    public Optional<List<ObjectDefinition>> getTypesFromInterfaceOrUnion(String name) {
+if (isUnion(name)) {
+return Optional.of(getUnionSubTypes(isConnectionObject(name) ? getConnectionObject(name).getNodeType() : name));
+}
+if (isInterface(name)) {
+return Optional.of(getImplementationsForInterface(isConnectionObject(name) ? getConnectionObject(name).getNodeType() : name));
+}
+return Optional.empty();
+}

	if (multitableName.equalsIgnoreCase(NODE_TYPE.getName()) \|\| multitableName.equalsIgnoreCase(ERROR_TYPE.getName())) {
	if (multitableName.equalsIgnoreCase(ERROR_TYPE.getName())) {

	addErrorMessage(MISSING_TABLE_ON_MULTITABLE.getMsg(), typesMissingTable, field.getContainerTypeName(), field.getName(), field.getTypeName());
	addErrorMessage(MISSING_TABLE_ON_MULTITABLE.getMsg(), typesMissingTable, field.formatPath(), field.getTypeName());

	MISSING_TABLE_ON_MULTITABLE("Type(s) '%s' are used in a query '%s.%s' returning multitable interface or union '%s', but do not have tables set. This is not supported.");
	MISSING_TABLE_ON_MULTITABLE("Type(s) '%s' are used in a query '%s' returning multitable interface or union '%s', but do not have tables set. This is not supported.");

Conversation

Erkelinux commented Feb 12, 2026

Uh oh!

andreahn commented Feb 16, 2026

Uh oh!

Erkelinux commented Mar 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants