From 09d35c15fab53616934738543fe3b947bcb78262 Mon Sep 17 00:00:00 2001 From: delchev Date: Sun, 16 Aug 2026 01:30:18 +0300 Subject: [PATCH] fix(sdk): findById answers null for an absent id, as documented (#6420) JavaRepository.findById documented "the entity, or null if not found" but delegated to a store method that threw IllegalArgumentException instead, so the documented null never happened and every findById + null-guard was dead code. Callers who followed the javadoc surfaced an absent id as a 500: a controller looking a record up by a path parameter never reached its own 404, a posting handler could not skip a source row deleted between the event and the handler, and a dangling FK in a schedules-notify job killed the whole run instead of skipping one row. The whole generated corpus was written against the documented contract - the glue templates and the generated DAO/REST templates all null-guard the result - so the honest fix is to honour it. An absent id is an ordinary outcome of a lookup, and the caller owns what it means: findOne(id) stays the Optional sibling for a caller that wants to chain its own failure (orElseThrow carrying a 404), and the generated controllers already read single records through it. Co-Authored-By: Claude Opus 5 --- .../store/java/repository/JavaRepository.java | 10 +- .../store/java/store/JavaEntityStore.java | 10 +- components/engine/engine-java/CLAUDE.md | 2 + .../tests/api/JavaRepositoryFindByIdIT.java | 108 ++++++++++++++++++ .../tables/thing.table | 19 +++ .../things/Thing.java | 31 +++++ .../things/ThingController.java | 57 +++++++++ .../things/ThingRepository.java | 21 ++++ 8 files changed, 251 insertions(+), 7 deletions(-) create mode 100644 tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/JavaRepositoryFindByIdIT.java create mode 100644 tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/tables/thing.table create mode 100644 tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/Thing.java create mode 100644 tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/ThingController.java create mode 100644 tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/ThingRepository.java diff --git a/components/data/data-store-java/src/main/java/org/eclipse/dirigible/components/data/store/java/repository/JavaRepository.java b/components/data/data-store-java/src/main/java/org/eclipse/dirigible/components/data/store/java/repository/JavaRepository.java index 16461ab6bb8..6258a4488aa 100644 --- a/components/data/data-store-java/src/main/java/org/eclipse/dirigible/components/data/store/java/repository/JavaRepository.java +++ b/components/data/data-store-java/src/main/java/org/eclipse/dirigible/components/data/store/java/repository/JavaRepository.java @@ -111,7 +111,11 @@ public int updateProperties(Object id, Map values) { } /** - * Look up an entity by primary key. + * Look up an entity by primary key. An absent id is an ordinary outcome — a dangling foreign key an + * event handler should skip, a path parameter a controller should answer {@code 404} for — so it + * reads back as {@code null} rather than as a thrown exception. Callers that require the row to + * exist use {@link #findOne(Object)} and choose their own failure, e.g. + * {@code findOne(id).orElseThrow(() -> new ResponseStatusException(NOT_FOUND))}. * * @param id the primary-key value * @return the entity, or {@code null} if not found @@ -121,7 +125,9 @@ public T findById(Object id) { } /** - * Look up an entity by primary key. + * Look up an entity by primary key — the {@link Optional} variant of {@link #findById(Object)}, for + * callers that chain the absent case (an {@code orElseThrow} carrying their own status, an + * {@code orElseGet} default). * * @param id the primary-key value * @return an optional carrying the entity if it exists diff --git a/components/data/data-store-java/src/main/java/org/eclipse/dirigible/components/data/store/java/store/JavaEntityStore.java b/components/data/data-store-java/src/main/java/org/eclipse/dirigible/components/data/store/java/store/JavaEntityStore.java index cee516b01c1..a329aa0691f 100644 --- a/components/data/data-store-java/src/main/java/org/eclipse/dirigible/components/data/store/java/store/JavaEntityStore.java +++ b/components/data/data-store-java/src/main/java/org/eclipse/dirigible/components/data/store/java/store/JavaEntityStore.java @@ -206,17 +206,17 @@ public int updateProperties(Class type, Object id, Map va private static final Pattern PLAIN_PROPERTY = Pattern.compile("[A-Za-z_][A-Za-z0-9_]*"); /** - * Find by id. Throws {@link IllegalArgumentException} when not found — use {@link #findOne} for the - * optional variant. + * Find by id, or {@code null} when there is no such row — an absent id is an ordinary outcome of a + * lookup, not a failure. Callers that require the row to exist use {@link #findOne} and decide the + * failure themselves (a {@code 404} at a controller boundary, a skip in an event handler). * * @param the entity type * @param type the entity class * @param id the primary key - * @return the entity + * @return the entity, or {@code null} if not found */ public T findById(Class type, Object id) { - return findOne(type, id).orElseThrow( - () -> new IllegalArgumentException("No entity [" + type.getSimpleName() + "] with id [" + id + "]")); + return findOne(type, id).orElse(null); } /** diff --git a/components/engine/engine-java/CLAUDE.md b/components/engine/engine-java/CLAUDE.md index 179360b0d24..288e745d701 100644 --- a/components/engine/engine-java/CLAUDE.md +++ b/components/engine/engine-java/CLAUDE.md @@ -154,6 +154,8 @@ lazily). `EntityBeanMapper` does bean↔map; `JavaEntityToHbmMapper` reflects an (shares `HbmXmlDescriptor` with `data-store` — audit both if you change either). SessionFactory roots at the default user-data datasource, not SystemDB. +**An absent id reads back as `null`, it does not throw.** `findById(id)` answers `null` when there is no such row, and `findOne(id)` is its `Optional` sibling — both are ordinary lookups, and the caller owns what absence means: a `404` at a controller boundary, a skip in an event handler. It used to throw `IllegalArgumentException` while its javadoc promised `null`, which made every documented `findById` + null-guard dead code — a controller looking a record up by a path parameter answered `500` for an unknown id, and a handler meant to skip a dangling FK failed its whole run instead (issue #6420; the generated glue and DAO templates were all written against the documented contract). Do NOT reintroduce a throwing lookup: a caller who needs "must exist" writes `findOne(id).orElseThrow(...)` and chooses its own failure. `JavaRepositoryFindByIdIT` covers it end-to-end. + **A large-text column needs `@Lob` — the mapping resizes the column to whatever it claims.** Entity registration runs Hibernate's `hbm2ddl.auto = update`, which does not only create missing tables: it ALTERS an existing column to match the mapping. A plain `String` property claims `@Column(length = ...)`, whose default is **255**, so a `CLOB` / `TEXT` column declared by the project's `.table` silently became a `VARCHAR(255)` on every deploy (issue #6346's recurring "Incompatible change ... VARCHAR to be changed to CLOB" was the schema layer noticing). Annotate the property `@Lob` and it is mapped past the dialect's maximum `VARCHAR`, which resolves to the database's own large-text type (`CLOB` on H2, `TEXT` on PostgreSQL) and leaves the column alone. Do NOT try to pin the type with `@Column(columnDefinition = ...)` — the mapper ignores it, and a raw SQL type name is not portable across dialects anyway. Generated entities don't need `@Lob`: an intent `type: text` field is a `VARCHAR(4000)` whose length the generated `@Column` declares. `JavaEntityLobColumnIT` covers the contract end-to-end. **Manage entities ONLY through their generated `Repository` — NEVER the generic `Store`/`Database` for entity CRUD.** The generated repository (`@Repository extends JavaRepository`) is the *only* sanctioned way to load/save/update/delete a managed entity, because it carries validations, **event publishing** (`Producer.sendToTopic` on the create/`-updated`/`-deleted` topics that intent triggers/reactions/rollups/notifications listen on), the multilingual read-overlay (a `multilingual: true` entity's finds translate string properties from its `_LANG` table for the caller's `Accept-Language` via `org.eclipse.dirigible.sdk.db.Translator`), and other per-entity behaviour. The generic `org.eclipse.dirigible.sdk.db.Store` (name-keyed dynamic map) and raw `Database` SQL **bypass all of that silently** and MUST NOT be used to read or mutate a managed entity. (`updateWithoutEvent` is fine — it's a deliberate repository method that keeps `super.update`'s validations/i18n and only omits the event, for workflow-driven system writes: intent SetField/Writer/trigger delegates.) Consequence for a *reusable* delegate/service: it can't statically import a foreign `Entity`, so the code that touches a specific entity must live **in that entity's project** (where it imports that project's repository); keep only entity-agnostic helpers (e.g. a number generator over its own `NumberRepository`) in a shared project. Don't make code "general" by reaching into arbitrary entities through `Store`. diff --git a/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/JavaRepositoryFindByIdIT.java b/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/JavaRepositoryFindByIdIT.java new file mode 100644 index 00000000000..45e7b0d10ab --- /dev/null +++ b/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/JavaRepositoryFindByIdIT.java @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2010-2026 Eclipse Dirigible contributors + * + * All rights reserved. This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v2.0 which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v20.html + * + * SPDX-FileCopyrightText: Eclipse Dirigible contributors SPDX-License-Identifier: EPL-2.0 + */ +package org.eclipse.dirigible.integration.tests.api; + +import static io.restassured.RestAssured.given; +import static org.hamcrest.Matchers.containsString; + +import java.sql.Connection; +import java.sql.Statement; + +import org.eclipse.dirigible.components.data.sources.manager.DataSourcesManager; +import org.eclipse.dirigible.components.initializers.synchronizer.SynchronizationProcessor; +import org.eclipse.dirigible.repository.api.IRepository; +import org.eclipse.dirigible.tests.base.IntegrationTest; +import org.eclipse.dirigible.tests.base.ProjectUtil; +import org.eclipse.dirigible.tests.framework.restassured.RestAssuredExecutor; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; + +/** + * {@code JavaRepository.findById} answers {@code null} for an id that is not there, as its javadoc + * documents. + * + *

+ * It used to throw {@code IllegalArgumentException} instead, which made every documented + * {@code findById} + null-guard dead code: a controller looking a record up by a path parameter + * returned {@code 500} for an unknown id instead of its own {@code 404}, and an event handler meant + * to skip a dangling foreign key failed its whole run (issue #6420). + */ +class JavaRepositoryFindByIdIT extends IntegrationTest { + + private static final String PROJECT = "JavaRepositoryFindByIdIT"; + private static final String CONTROLLER = "/services/java/" + PROJECT + "/things/ThingController"; + private static final String TABLE_NAME = "FIND_BY_ID_THING"; + private static final int UNKNOWN_ID = 4242; + private static final long TIMEOUT_SECONDS = 30; + + @Autowired + private IRepository repository; + + @Autowired + private ProjectUtil projectUtil; + + @Autowired + private SynchronizationProcessor synchronizationProcessor; + + @Autowired + private RestAssuredExecutor restAssuredExecutor; + + @Autowired + private DataSourcesManager dataSourcesManager; + + @Test + void an_unknown_id_reads_back_as_null_not_as_a_failure() { + ClientJavaProjectDeployer.deploy(repository, projectUtil, synchronizationProcessor, PROJECT, PROJECT); + + // The controller's own 404 is reachable, on both the null-returning and the Optional variant. + // This is also the first call, so it retries until the freshly compiled route is registered. + assertResponse("/byId/" + UNKNOWN_ID, 404, "no such thing"); + assertResponse("/byOne/" + UNKNOWN_ID, 404, "no such thing"); + + // The stored record still comes back - the null is about absence, not about the whole lookup. + String id = seed(); + assertResponse("/byId/" + id, 200, "seeded"); + assertResponse("/byOne/" + id, 200, "seeded"); + } + + private String seed() { + return restAssuredExecutor.executeWithResult(() -> given().when() + .get(CONTROLLER + "/seed") + .then() + .statusCode(200) + .extract() + .asString() + .trim()); + } + + private void assertResponse(String path, int expectedStatus, String expectedFragment) { + restAssuredExecutor.execute(() -> given().when() + .get(CONTROLLER + path) + .then() + .statusCode(expectedStatus) + .body(containsString(expectedFragment)), + TIMEOUT_SECONDS); + } + + /** + * The fixture files go away with the Dirigible folder the base class wipes per test class; the + * table itself would survive a local run against an unclean target and carry its rows into the next + * one. + */ + @AfterEach + void dropTable() throws Exception { + try (Connection connection = dataSourcesManager.getDefaultDataSource() + .getConnection(); + Statement statement = connection.createStatement()) { + statement.execute("DROP TABLE IF EXISTS \"" + TABLE_NAME + "\""); + } + } +} diff --git a/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/tables/thing.table b/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/tables/thing.table new file mode 100644 index 00000000000..39637be4cc1 --- /dev/null +++ b/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/tables/thing.table @@ -0,0 +1,19 @@ +{ + "name": "FIND_BY_ID_THING", + "type": "TABLE", + "columns": [ + { + "type": "INTEGER", + "primaryKey": true, + "identity": true, + "nullable": false, + "name": "THING_ID" + }, + { + "type": "VARCHAR", + "length": "255", + "nullable": true, + "name": "THING_NAME" + } + ] +} diff --git a/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/Thing.java b/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/Thing.java new file mode 100644 index 00000000000..cf2368b6e96 --- /dev/null +++ b/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/Thing.java @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2010-2026 Eclipse Dirigible contributors + * + * All rights reserved. This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v2.0 which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v20.html + * + * SPDX-FileCopyrightText: Eclipse Dirigible contributors SPDX-License-Identifier: EPL-2.0 + */ +package things; + +import org.eclipse.dirigible.sdk.db.Column; +import org.eclipse.dirigible.sdk.db.Entity; +import org.eclipse.dirigible.sdk.db.GeneratedValue; +import org.eclipse.dirigible.sdk.db.GenerationType; +import org.eclipse.dirigible.sdk.db.Id; +import org.eclipse.dirigible.sdk.db.Table; + +/** Maps the same table as tables/thing.table. */ +@Entity +@Table(name = "FIND_BY_ID_THING") +public class Thing { + + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + @Column(name = "THING_ID") + public Integer id; + + @Column(name = "THING_NAME") + public String name; +} diff --git a/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/ThingController.java b/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/ThingController.java new file mode 100644 index 00000000000..b6abb2e5a13 --- /dev/null +++ b/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/ThingController.java @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2010-2026 Eclipse Dirigible contributors + * + * All rights reserved. This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v2.0 which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v20.html + * + * SPDX-FileCopyrightText: Eclipse Dirigible contributors SPDX-License-Identifier: EPL-2.0 + */ +package things; + +import org.eclipse.dirigible.sdk.http.Controller; +import org.eclipse.dirigible.sdk.http.Get; +import org.eclipse.dirigible.sdk.http.PathParam; +import org.eclipse.dirigible.sdk.http.Response; + +/** + * The shape the SDK javadoc documents: look a record up by a path parameter and answer 404 when it + * is not there. The null guard is only reachable if findById actually returns null for an unknown + * id - a throw would make the same request a 500. + */ +@Controller +public class ThingController { + + private final ThingRepository things; + + public ThingController(ThingRepository things) { + this.things = things; + } + + @Get("/seed") + public String seed() { + Thing thing = new Thing(); + thing.name = "seeded"; + return String.valueOf(things.save(thing).id); + } + + @Get("/byId/{id}") + public String byId(@PathParam("id") Integer id) { + Thing thing = things.findById(id); + if (thing == null) { + Response.setStatus(404); + return "no such thing"; + } + return thing.name; + } + + @Get("/byOne/{id}") + public String byOne(@PathParam("id") Integer id) { + return things.findOne(id) + .map(thing -> thing.name) + .orElseGet(() -> { + Response.setStatus(404); + return "no such thing"; + }); + } +} diff --git a/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/ThingRepository.java b/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/ThingRepository.java new file mode 100644 index 00000000000..3cadafd52d7 --- /dev/null +++ b/tests/tests-integrations/src/main/resources/JavaRepositoryFindByIdIT/things/ThingRepository.java @@ -0,0 +1,21 @@ +/* + * Copyright (c) 2010-2026 Eclipse Dirigible contributors + * + * All rights reserved. This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v2.0 which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v20.html + * + * SPDX-FileCopyrightText: Eclipse Dirigible contributors SPDX-License-Identifier: EPL-2.0 + */ +package things; + +import org.eclipse.dirigible.components.data.store.java.repository.JavaRepository; +import org.eclipse.dirigible.sdk.component.Repository; + +@Repository +public class ThingRepository extends JavaRepository { + + public ThingRepository() { + super(Thing.class); + } +}