rzo1 commented on PR #144: URL: https://github.com/apache/openjpa/pull/144#issuecomment-5325916594
Here is a table @solomax for better overview, with links into each discussion. Might be helpful — we can update it automatically if needed. 56 unresolved review threads, excluding the `EnumValueHandler` `@EnumeratedValue` thread currently being worked on. | ID | Link | Title | Comment | Author | | --- | --- | --- | --- | --- | | 3426797320 | [3426797320](https://github.com/apache/openjpa/pull/144#discussion_r3426797320) | Silently swallowed no-op batched delete result | is this by spec? how do I know my delete was a noop as a caller now? | rmannibucau | | 3426807128 | [3426807128](https://github.com/apache/openjpa/pull/144#discussion_r3426807128) | Null state manager cases should throw instead of skip | wonder if these null cases shouldn't throw, means enhancement is broken or setup (agent) is broken no? | rmannibucau | | 3426812145 | [3426812145](https://github.com/apache/openjpa/pull/144#discussion_r3426812145) | Possible missed loop case in row flush handling | same, to review if we don't loop a case there | rmannibucau | | 3682999074 | [3682999074](https://github.com/apache/openjpa/pull/144#discussion_r3682999074) | New JPQL keywords no longer usable as aliases | **(medium)** `identification_variable()` only accepts `<IDENTIFIER>`, so every newly introduced token (ID, VERSION, RIGHT, ON, NULLS, FIRST, LAST, CAST, STRING, UNION, INTERSECT, EXCEPT, TREAT, ...) can no longer be used as an identification variable or result alias. Existing queries like `SELECT e.id AS id ... ORDER BY id` or aliases named `first`/`on` now fail to parse - a backward-compat regression worth documenting or mitigating with soft keywords. (re line 1560, outside the diff hunks) | rmannibucau | | 3682999176 | [3682999176](https://github.com/apache/openjpa/pull/144#discussion_r3682999176) | ID()/VERSION() limited to equality against parameters | **(medium)** `entity_id_or_version_comp()` only allows `ID(x)`/`VERSION(x)` compared with `=`/`<>` against an input parameter. Spec-legal forms like `ID(e) = 5`, `VERSION(e) >= :v` or `ID(a) = ID(b)` do not parse - intentional first step, or should these functions be reachable from the general comparison/arithmetic productions? | rmannibucau | | 3682999345 | [3682999345](https://github.com/apache/openjpa/pull/144#discussion_r3682999345) | Broadened auto-flush before every query hurts perf | **(medium)** With FLUSH_TRUE (default FlushModeType.AUTO) this now flushes before every query as soon as anything in the context is dirty, and the `flush == FLUSH_TRUE` addition at line 1033 also overrides the IgnoreChanges setting. That defeats the access-path optimization and can be a real perf regression in write-heavy transactions - can the broadened check be limited to types related to the query access path, or guarded by a compatibility flag? | rmannibucau | | 3682999520 | [3682999520](https://github.com/apache/openjpa/pull/144#discussion_r3682999520) | Full context flush after in-memory bulk update/delete | **(medium)** The new `_broker.flush()` after in-memory bulk delete (and update at line 1156) flushes the entire persistence context as a side effect of `executeUpdate()`, changing when unrelated pending changes hit the database (locks, triggers, constraint timing). Is flushing only the affected instances feasible, or is the full flush deliberate? | rmannibucau | | 3682999635 | [3682999635](https://github.com/apache/openjpa/pull/144#discussion_r3682999635) | Transient object wrongly assumed detached in flush check | **(medium)** This early-return treats any unmanageable instance of a known entity type as detached without verifying it was ever persisted, so a truly transient unenhanced object referenced without cascade now passes the check and fails later (or silently persists a broken FK). Line 824 also adds `_broker.isDetached(obj, true)`, a DB round-trip inside the flush path for every manageable object without a state manager. Can both paths at least verify a non-null/assigned identity before assuming detached? | rmannibucau | | 3682999733 | [3682999733](https://github.com/apache/openjpa/pull/144#discussion_r3682999733) | Embedded converter override leaks to shared embeddable meta | **(medium)** `propagateEmbeddedConverters()` sets the converter on the shared embeddable ClassMetaData. If two entities embed the same embeddable but only one declares `@Convert(attributeName=...)`, the override leaks to all other usages (last-resolved wins). Should the override only apply to the per-embedding `embeddedMeta` copy? | rmannibucau | | 3682999838 | [3682999838](https://github.com/apache/openjpa/pull/144#discussion_r3682999838) | Unsafe publication of cached AttributeConverter instance | **(medium)** The converter instance is now created once and cached in a non-volatile field without synchronization; FieldMetaData is shared across brokers/threads, so this is unsafe publication and a behavior change (previously a new converter instance per conversion, now one shared instance that must be thread-safe). Consider thread-safe caching and documenting that AttributeConverters are treated as shared/stateless. | rmannibucau | | 3683000132 | [3683000132](https://github.com/apache/openjpa/pull/144#discussion_r3683000132) | extractSchemaGenObjects mutates caller's properties map | **(medium)** `extractSchemaGenObjects` calls `map.remove(key)` on the caller-supplied properties map - this mutates user configuration and throws UnsupportedOperationException for unmodifiable maps (e.g. `Map.of(...)` passed to `createEntityManagerFactory`). Can the Writer/Reader values be captured without mutating the input? | rmannibucau | | 3683000248 | [3683000248](https://github.com/apache/openjpa/pull/144#discussion_r3683000248) | In-memory ID(), set ops and nullPrecedence unimplemented | **(medium)** `getNativeObjectId` returns the same `GetObjectId` as `getObjectId`, which evaluates to the internal ObjectId wrapper (e.g. LongId), not the raw PK value - so an in-memory `ID(e) = :id` comparison against the plain key may never match; should it unwrap like the JDBC side? Similarly `setOperands`/`setOperationType` and `nullPrecedence` appear consumed only by the JDBC store, so in-memory execution of UNION/INTERSECT/EXCEPT or NULLS FIRST/LAST silently produces wrong results - should the in-memory path reject or implement them? | rmannibucau | | 3683000358 | [3683000358](https://github.com/apache/openjpa/pull/144#discussion_r3683000358) | hasEmbeddableAnnotation relies on annotation simple name | **(low)** `hasEmbeddableAnnotation` matches any annotation whose simple name is "Embeddable" (from any package) and misses embeddables declared only in orm.xml - would checking the repository metadata (e.g. `fmd.getEmbeddedMetaData()`) be more reliable than reflection on annotation names? | rmannibucau | | 3683000504 | [3683000504](https://github.com/apache/openjpa/pull/144#discussion_r3683000504) | Direct access to ImplHelper._unenhancedInstanceMap | **(low)** Reaching into `ImplHelper._unenhancedInstanceMap` (a public mutable static field) directly is fragile - suggest a small `ImplHelper.registerUnenhancedInstance(obj, pc)` method instead of exposing the raw map. | rmannibucau | | 3683001426 | [3683001426](https://github.com/apache/openjpa/pull/144#discussion_r3683001426) | dropExcludedTypeTables drops user tables with raw SQL | **(high)** `dropExcludedTypeTables` issues a raw `"DROP TABLE " + tableName` bypassing the dictionary (no identifier quoting/`toDBName`, no CASCADE handling, will fail on Postgres with dependent constraints) and swallows every exception at trace level. More fundamentally, dropping a user table because its type was excluded from synchronization is destructive - an excluded type may be a table managed externally on purpose. Is this only here to make a specific test pass? It probably should not ship in production code. | rmannibucau | | 3683001538 | [3683001538](https://github.com/apache/openjpa/pull/144#discussion_r3683001538) | NULLS FIRST/LAST string rewrite breaks on commas, triplicated | **(high)** The NULLS FIRST/LAST emulation finds the last order term with `sql.lastIndexOf(", ", termDirStart)`, so any ORDER BY expression containing a comma (e.g. `COALESCE(t0.x, 0) DESC`) is split mid-argument-list and the rewrite produces corrupt SQL; duplicating the expression also duplicates `?` markers without duplicating bound parameters. The block is triplicated in MariaDBDictionary (~575) and SQLServerDictionary (~484) - can it be extracted to a shared helper operating on the order term before it is appended to the buffer instead of string-parsing it back out? | rmannibucau | | 3683001854 | [3683001854](https://github.com/apache/openjpa/pull/144#discussion_r3683001854) | TREAT discriminator filter excludes subclasses of target | **(medium)** `appendTreatDiscriminator` emits `disc = <treated class value>` only. Per JPA, `TREAT(x AS Middle)` must also match subtypes of Middle, so for a 3-level hierarchy this incorrectly filters out instances of Middle's subclasses - it should be an IN over the discriminator values of the treated class and all mapped subclasses. It also only handles `cols[0]` of the discriminator. | rmannibucau | | 3683001994 | [3683001994](https://github.com/apache/openjpa/pull/144#discussion_r3683001994) | Schema-gen methods leak a Broker just to get classloader | **(medium)** `createPersistenceStructure`, `dropPersistenceStrucuture`, `validatePersistenceStruture` and `truncateData` each create a Broker via `super.newBrokerImpl(...)` that is only used for `getClassLoader()` and never closed - a broker (and its resources) leaked per call. Can the classloader be obtained without instantiating a broker? | rmannibucau | | 3683002126 | [3683002126](https://github.com/apache/openjpa/pull/144#discussion_r3683002126) | Static _droppedTables shared across concurrent EMFs | **(medium)** `_droppedTables` is JVM-global static mutable state and `clearDroppedTables()` is invoked from JDBCBrokerFactory whenever any EMF with schema-gen properties spins up - with two persistence units initializing concurrently (common in app servers) one EMF wipes the other's in-flight tracking. Matching also uses `toUpperCase()` without `Locale.ROOT` (here, line 1239 and 1519-1529) and compares a full identifier against a name regex-stripped from raw DDL, so schema-qualified or quoted names will not match. Could this state live on the configuration instead? | rmannibucau | | 3683002267 | [3683002267](https://github.com/apache/openjpa/pull/144#discussion_r3683002267) | Range ignored in executeSetOperatorQuery | **(medium)** `executeSetOperatorQuery` receives `range` but never uses it, so `setFirstResult`/`setMaxResults` on a UNION/INTERSECT/EXCEPT query are silently ignored. Should the range at least be applied via RangeResultObjectProvider like the normal path does? | rmannibucau | | 3683002420 | [3683002420](https://github.com/apache/openjpa/pull/144#discussion_r3683002420) | Bulk delete no longer cleans dependent/collection rows | **(medium)** Removing the `getCascadeDelete() != CASCADE_NONE -> INVALID` guard means bulk DELETE no longer falls back to loading instances for entities with cascading/dependent fields. JPA 4.10 (no cascade on bulk delete) is fine, but this strategy also handled dependent-field cleanup: join-table / element-collection rows previously removed via the in-memory fallback can now be left orphaned unless the DB has ON DELETE CASCADE. Was that trade-off verified for the element-collection case? | rmannibucau | | 3683002577 | [3683002577](https://github.com/apache/openjpa/pull/144#discussion_r3683002577) | Long cast uses decimalTypeName instead of bigint | **(medium)** Casting to Long uses `dict.decimalTypeName`; on MySQL `CAST(x AS DECIMAL)` defaults to DECIMAL(10,0), so large long values overflow/truncate - why not `bigintTypeName` for the long case? Also `getDbNumberTargetTypeName` sanitizes the `{0}` size suffix while the sibling TypecastAsString.java:152 appends `dict.varcharTypeName` raw - the two siblings should share the same sanitize logic. | rmannibucau | | 3683002730 | [3683002730](https://github.com/apache/openjpa/pull/144#discussion_r3683002730) | VersionVal NPEs on surrogate or missing version | **(medium)** `getColumns()` NPEs when the entity has a surrogate version or no version at all (`getVersionFieldMapping()` returns null), so `VERSION(e)` on such an entity dies with NullPointerException instead of a meaningful error - `initialize` should validate this like it validates the class mapping. The error at line 99 also reuses the `bad-getobjectid` message, which is misleading for a VERSION() failure. | rmannibucau | | 3683003006 | [3683003006](https://github.com/apache/openjpa/pull/144#discussion_r3683003006) | IdClass paths swallow exceptions and rely on field order | **(medium)** The IdClass reconstruction/extraction paths `catch (Exception)` and silently continue (here the field stays null; in `toDataStoreValue` at line 227 the PK columns get nulls written). Swallowing exceptions around primary-key values risks silently persisting/loading corrupt identities - at minimum a warn log, arguably a StoreException. Also `getInstanceFields` (line 448) maps IdClass fields to columns by `getDeclaredFields()` order, which the JVM does not guarantee - matching by name would be safer. | rmannibucau | | 3683003132 | [3683003132](https://github.com/apache/openjpa/pull/144#discussion_r3683003132) | Instant.now() precision loss on DB round-trip | **(medium)** `Instant.now()` carries micro/nano precision on modern JVMs; if the version column's precision is lower (MySQL TIMESTAMP defaults, Oracle DATE) the value read back differs from the in-memory version, producing spurious optimistic-lock failures on the next flush - the same reason TimestampVersionStrategy deliberately uses millisecond granularity. Should this truncate to a precision known to survive the DB round-trip? | rmannibucau | | 3683003222 | [3683003222](https://github.com/apache/openjpa/pull/144#discussion_r3683003222) | Null named parameter falls through to positional lookup | **(medium)** Using `userParams.get(name) == null` to fall through to positional lookup means an explicitly bound null named parameter is silently replaced by whatever is registered under the position key - `containsKey` should distinguish "bound to null" from "absent". Related: the new `c.getIndex() < params.length` guards at lines 188/195 silently skip binding instead of failing, turning a caller bug into an unbound-parameter SQLException far from the cause. | rmannibucau | | 3683003316 | [3683003316](https://github.com/apache/openjpa/pull/144#discussion_r3683003316) | storeCharsAsNumbers default flip breaks existing schemas | **(medium)** Flipping `storeCharsAsNumbers` to false for PostgreSQL >= 9 changes the default mapping for existing applications: char fields previously stored in INTEGER columns will now map/validate as CHAR, so schemas created by older OpenJPA versions fail validation or read wrongly after an upgrade. Intended compatibility break? Should be release-noted and overridable. | rmannibucau | | 3683003416 | [3683003416](https://github.com/apache/openjpa/pull/144#discussion_r3683003416) | Ceiling mutates shared operator field in appendTo | **(low)** `appendTo` mutates the instance field `operator` before delegating to `super.appendTo` (same pattern in NaturalLogarithm.java:49). Compiled query plans are cached and shared, so this is a data race on shared state; passing the dictionary function down locally (e.g. an `appendTo(..., String operator)` overload in UnaryOp) would keep the Val immutable. | rmannibucau | | 3683003654 | [3683003654](https://github.com/apache/openjpa/pull/144#discussion_r3683003654) | Converter instantiated reflectively, skips CDI and nulls | **(low)** Two questions: (1) the converter is instantiated via `getDeclaredConstructor().newInstance()`, bypassing CDI-managed converters (JPA allows converters as CDI beans with injection); (2) why reflection/`findMethod` instead of casting to `jakarta.persistence.AttributeConverter` and calling it directly? Note also the `val == null` short-circuits mean a converter mapping null to a default value is never consulted for nulls. | rmannibucau | | 3683004301 | [3683004301](https://github.com/apache/openjpa/pull/144#discussion_r3683004301) | String cache mode properties no longer converted to enum | **(medium)** The early `return (T) value;` now also fires for String values of `cache.retrieveMode`/`cache.storeMode`, skipping the enum conversion below. Since EntityManagerImpl now exposes `setCacheRetrieveMode(CacheRetrieveMode)`, `em.setProperty("jakarta.persistence.cache.retrieveMode", "USE")` will try to inject a raw String into the enum setter and fail, whereas the old code converted it. Should Strings still be converted to the target enum here? | rmannibucau | | 3683004426 | [3683004426](https://github.com/apache/openjpa/pull/144#discussion_r3683004426) | treat() join overloads silently cast without narrowing | **(medium)** Only `treat(Root)` gets a real implementation (`RootImpl.TreatedRoot`); all the join overloads (lines 429-450) and `treat(Path)` (line 458) just cast and return the same object. That is a silent no-op: no type narrowing is applied, but instead of the previous explicit UnsupportedOperationException users now get wrong behavior with no diagnostic. Could the unsupported overloads keep throwing (or get a TreatedJoin analogous to TreatedRoot)? | rmannibucau | | 3683004555 | [3683004555](https://github.com/apache/openjpa/pull/144#discussion_r3683004555) | version 3.1 persistence.xml validated against 3.0 XSD | **(medium)** Documents declaring `version="3.1"` are routed to `persistence_3_0.xsd.rsrc`, but that schema declares `version` as `fixed="3.0" use="required"`, so such documents always fail XSD validation - and with the new SAX-rethrow logic at line 570 this now aborts unit discovery instead of being skipped. Is the branch intentional, and should not it validate against a schema that accepts "3.1"? (Note the copy-pasted `PERSISTENCE_XSD_3_0` in the 3.1 condition.) | rmannibucau | | 3683004644 | [3683004644](https://github.com/apache/openjpa/pull/144#discussion_r3683004644) | convert() mutates caller config, ignores mappingFiles | **(medium)** `convert(PersistenceConfiguration)` mutates the caller's configuration object (`config.property(...)` for MetaDataFactory and noPersistenceXMLResource) - a surprising side effect for a converter, and repeated `createEntityManagerFactory(config)` calls keep appending. It also ignores `config.mappingFiles()` and `config.qualifiers()` entirely, and `setPersistenceUnitName(config.name())` is called twice (lines 648 and 662). Could the OpenJPA-specific properties go into the returned map instead, and mapping files be wired through? | rmannibucau | | 3683004763 | [3683004763](https://github.com/apache/openjpa/pull/144#discussion_r3683004763) | Typos, stray semicolon, and validate() masking unsupported | **(medium)** Nits before this freezes: the delegated `BrokerFactory` method names carry the `Strucuture`/`Struture` typos, `truncateData();;` has a double semicolon at line 64, and the `(Exception) ex` cast at line 58 is redundant. Also `validate()` wraps even UnsupportedOperationException from stores that do not implement validation into `SchemaValidationException("Schema could not be validated: null")`, misreporting a missing capability as a validation failure. | rmannibucau | | 3683004886 | [3683004886](https://github.com/apache/openjpa/pull/144#discussion_r3683004886) | getReference(entity) fails on composite ids | **(medium)** `getReference(T entity)` only extracts the PK when `pkFields.length == 1`; for composite/IdClass/EmbeddedId entities `pk` stays null and the call fails downstream with a misleading "null pk" IllegalArgumentException. Also `entity.getClass()` (line 2657) may be a runtime subclass without direct metadata. Could composite ids be supported via `broker.getObjectId`, or at least fail with an explicit "composite id not supported" message? | rmannibucau | | 3683005009 | [3683005009](https://github.com/apache/openjpa/pull/144#discussion_r3683005009) | addNamedQuery relabels criteria query as JPQL | **(medium)** `addNamedQuery` re-labels a criteria query as JPQL using `queryImpl.getQueryString()`, which for criteria queries is the CQL/toString rendering - OpenJPA has never guaranteed that string is parseable JPQL (parameter rendering, literals, treated paths). Has this been exercised beyond simple queries? The three `catch (Exception) { // ignore }` blocks below (lines 512-540) would also hide genuine failures - they could be narrowed to IllegalStateException where the spec defines it. | rmannibucau | | 3683005121 | [3683005121](https://github.com/apache/openjpa/pull/144#discussion_r3683005121) | Dead access-type annotation helper methods | **(medium)** `hasMixedAnnotations`, `hasFieldStrategyAnnotations` (547) and `hasGetterStrategyAnnotations` (556) are never called from anywhere - dead code from an earlier iteration of the access-type rework; suggest removing. | rmannibucau | | 3683005250 | [3683005250](https://github.com/apache/openjpa/pull/144#discussion_r3683005250) | getProperties() caches map missing EM-level defaults | **(low)** The old else branch seeding `getProperties()` from a throwaway EM was removed, so the result now depends on whether an EM was created before the first call - and since the map is cached, the EM-level defaults are then permanently missing. Intentional, or should the cache be invalidated once `emEmptyPropsProperties` becomes available? | rmannibucau | | 3683005413 | [3683005413](https://github.com/apache/openjpa/pull/144#discussion_r3683005413) | Locale-sensitive toUpperCase on temporal field name | **(low)** `field.toString().toUpperCase()` is locale-sensitive ("minute" breaks under a Turkish default locale). Prefer `toUpperCase(Locale.ROOT)`, and ideally key off the known LocalDateField/LocalTimeField constants rather than `toString()`. | rmannibucau | | 3683005700 | [3683005700](https://github.com/apache/openjpa/pull/144#discussion_r3683005700) | Dropped non-Serializable IdClass warning | **(low)** The "IdClass does not implement Serializable" warning was silently dropped both here and in AnnotationPersistenceMetaDataParser. Deliberate (3.2 relaxes it?) or lost in the rewrite? | rmannibucau | | 3683005795 | [3683005795](https://github.com/apache/openjpa/pull/144#discussion_r3683005795) | Mandatory JPA 3.2 methods throw UnsupportedOperationException | **(low)** `getNamedQueries(Class)` still throws UnsupportedOperationException, as do `EntityManagerImpl.createQuery(TypedQueryReference)` (EntityManagerImpl.java:2779) and `find(EntityGraph, Object, FindOption...)` (EntityManagerImpl.java:2645). These are mandatory JPA 3.2 API - planned before merge, or tracked in a follow-up JIRA? Worth referencing the issue in the exception message. | rmannibucau | | 3683006065 | [3683006065](https://github.com/apache/openjpa/pull/144#discussion_r3683006065) | setTimeout(null) cannot clear a previously set query timeout | **(low)** `setTimeout(null)` is silently ignored, so once a timeout is set it can never be cleared through this API (and `getTimeout()` keeps returning the stale value). Should null reset the fetch plan's query timeout to its default? | rmannibucau | | 3683006222 | [3683006222](https://github.com/apache/openjpa/pull/144#discussion_r3683006222) | Externalized-parameter tests reduced to no-op assertions | **(high)** These three tests kept their names ("CanDetectExternalized...") but no longer detect anything: the `getExpressions()` helper was deleted and the `isUsingExternalizedParameter(...)` assertions were replaced by `assertNotNull(getResultList())`, which can never fail. Was externalized-parameter detection actually removed from the prepared-query cache, or can the original assertions be restored? As written the tests are no-ops. | rmannibucau | | 3683006359 | [3683006359](https://github.com/apache/openjpa/pull/144#discussion_r3683006359) | LOCAL TIME test query is a tautology matching all rows | **(high)** The query in testGetCurrentLocalTime was changed to `localTimeField < LOCAL TIME OR localTimeField >= LOCAL TIME`, a tautology matching every row no matter what LOCAL TIME evaluates to. The test now only verifies the query parses. Could we keep an assertion that actually constrains the result (e.g. compare against a value persisted just before)? | rmannibucau | | 3683006506 | [3683006506](https://github.com/apache/openjpa/pull/144#discussion_r3683006506) | Results always materialized, lazy ResultList behavior lost | **(high)** Several assertions here (and in TestQueryTimeout) were inverted from "iterator must be invalid after query/EM close" to "iterator still works because results are now an ArrayList snapshot". Which JPA 3.2 clause requires this? More importantly, does this mean the lazy ResultList (openjpa.FetchBatchSize streaming results) is gone and results are always fully materialized? That would be a significant memory/perf behavior change deserving explicit discussion and release-noting, not just adjusted tests. | rmannibucau | | 3683006632 | [3683006632](https://github.com/apache/openjpa/pull/144#discussion_r3683006632) | Bulk delete no longer cleans join-table rows | **(high)** testSingleDelete/testBulkDelete were inverted from "addresses deleted" to "addresses remain" citing spec 4.10 - but that clause has said bulk delete does not cascade since JPA 1.0, so this is a deliberate break with long-standing OpenJPA behavior rather than something new in 3.2. Also the `assertSQL("DELETE FROM .*J_PERSON_ADDRESSES .*")` assertions were dropped entirely: are the join-table rows still cleaned up, or do we now leave dangling rows pointing at deleted pks (FK violation on constrained schemas)? Please keep an assertion on the join-table state and consider a compatibility option plus release note. Same change in TestBulkJPQLAndDataCache.java:122. | rmannibucau | | 3683006744 | [3683006744](https://github.com/apache/openjpa/pull/144#discussion_r3683006744) | Deeply nested multiselect extension now rejected | **(medium)** testDeeplyNestedShape previously verified OpenJPA's documented extension supporting arbitrary nesting of tuple/array selections (the old comment even said the negative test was retired for that reason); it is now inverted to expect IllegalArgumentException. The spec's "must not" has been there since 2.0, so this drops a working extension existing applications may rely on. Intentional, and should it be release-noted? | rmannibucau | | 3683006891 | [3683006891](https://github.com/apache/openjpa/pull/144#discussion_r3683006891) | Non-entity classes in persistence.xml silently skipped | **(medium)** Inverted from "listing a non-persistent class in persistence.xml raises ArgumentException" to "non-entity classes are silently skipped". Silent skipping also hides real user errors (forgotten annotations, broken enhancement) that previously failed fast. Is this required by a specific TCK test? If so, could we at least keep a warning log and reference the TCK requirement in a comment? | rmannibucau | | 3683007048 | [3683007048](https://github.com/apache/openjpa/pull/144#discussion_r3683007048) | Renamed getaXxx accessors drop JavaBeans naming coverage | **(medium)** The accessors were renamed from the JavaBeans-Introspector style (getaCAPITAL/getaWord/isaBoolean for fields aCAPITAL/aWord/aBoolean) to getACAPITAL/getAWord/isABoolean. This test existed precisely to cover the former naming, so property-access entities using IDE-generated getaXxx accessors would silently stop being recognized. Did 3.2 change property-name resolution, or is this adapting the test to a regression? Could both variants stay covered? | rmannibucau | | 3683007155 | [3683007155](https://github.com/apache/openjpa/pull/144#discussion_r3683007155) | Map-key column default KEY0 to entityCs_KEY breaks upgrades | **(medium)** Changing the expected default map-key column from KEY/KEY0 to entityCs_KEY (also TestContainerSpecCompatibilityOptions.java:426 and the KEY0 -> photos_KEY expected SQL in TestTypesafeCriteria) is spec-correct, but silently changes DDL/SQL against existing schemas created by older OpenJPA versions - upgrades will not find the KEY0 column. Should this be gated behind a compatibility option (this test class exists exactly for that) and called out in migration notes? | rmannibucau | | 3683007396 | [3683007396](https://github.com/apache/openjpa/pull/144#discussion_r3683007396) | assertSQL now ignores identifier delimiters suite-wide | **(medium)** assertSQL() now also matches after stripping identifier delimiters (same idea as the new same() helper in AbstractCriteriaTestCase.java:159). This globally relaxes every SQL assertion in the suite and would mask regressions in delimited-identifier handling. Could the delimiter-insensitive comparison be opt-in, or at least documented why it became necessary? | rmannibucau | | 3683007607 | [3683007607](https://github.com/apache/openjpa/pull/144#discussion_r3683007607) | testNotLoadedLazy duplicates eager check, lazy path untested | **(low)** In this rewrite, testNotLoadedLazy still calls `verifyIsLoadedEagerState(false)` (duplicate of testNotLoadedEager), so the lazy not-loaded path (`verifyIsLoadedLazyState(false)`) remains untested. Also createLazyEntity builds a RelEntity that is never persisted, which the new testLoadingLazyAttributeByName relies on. | rmannibucau | | 3683007780 | [3683007780](https://github.com/apache/openjpa/pull/144#discussion_r3683007780) | Deletion of TestSecurityContext needs rationale in PR | **(low)** This is the only deleted test file; deletion looks justified (SecurityManager removal on modern JDKs), but please state the rationale in the PR description so the removal is clearly intentional. | rmannibucau | | 3683007909 | [3683007909](https://github.com/apache/openjpa/pull/144#discussion_r3683007909) | MariaDB branch accepts either message without naming versions | **(low)** The MariaDB branch now accepts either the per-row or first-row failed object/message "across versions", weakening the exact-match check still applied to other DBs. If specific MariaDB versions differ, could the comment name the versions observed so this does not quietly absorb future regressions? | rmannibucau | | 3683008653 | [3683008653](https://github.com/apache/openjpa/pull/144#discussion_r3683008653) | Leftover [2024] bracket and stale CDDL sentence vs EFSL 1.1 | **(medium)** Two nits: the template brackets survived in "Copyright (c) [2024] Eclipse Foundation AISBL", and the next paragraph still says "OpenJPA elects to include this software in this distribution under the CDDL license" while the surrounding text was changed to EFSL 1.1 - should the CDDL sentence be updated for consistency? | rmannibucau | | 3683008798 | [3683008798](https://github.com/apache/openjpa/pull/144#discussion_r3683008798) | Oracle profile bind-mount jdbc_oradata outside the checkout | **(medium)** The oracle profile bind-mounts `${project.basedir}/../jdbc_oradata`, which for the root pom resolves outside the checkout (and to a different path per module since profiles are inherited); run-build-matrix.sh then pre-creates it with `chmod a+rwx` and warns that cleanup needs sudo. Would a named docker volume (or a path under `target/`) be cleaner so nothing leaks outside the repo? | rmannibucau | -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
