SAK-52193 Profile2 database error when updating profile info #14269
Conversation
WalkthroughThe SocialNetworkingInfo model class is modified to add a new persisted field Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
profile2/api/src/main/java/org/sakaiproject/profile2/api/model/SocialNetworkingInfo.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
profile2/api/src/main/java/org/sakaiproject/profile2/api/model/SocialNetworkingInfo.java
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (3)
profile2/api/src/main/java/org/sakaiproject/profile2/api/model/SocialNetworkingInfo.java (3)
21-22: LGTM! Necessary imports for lifecycle callbacks.The JPA lifecycle imports are correctly added to support the synchronization logic.
43-45: LGTM! Field correctly maps the legacy USER_UUID column.The field declaration properly addresses the database requirement with appropriate nullability constraint and clear documentation of its purpose.
57-60: LGTM! Constructor properly initializes both synchronized fields.The constructor correctly sets both
userIdanduserUuidto ensure initial consistency when entities are created via this constructor.
| // JPA lifecycle callback to ensure both columns are always synchronized | ||
| @PrePersist | ||
| @PreUpdate | ||
| private void syncUserIds() { | ||
| this.userUuid = this.userId; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the complete SocialNetworkingInfo.java file to understand the class structure
cat -n profile2/api/src/main/java/org/sakaiproject/profile2/api/model/SocialNetworkingInfo.javaRepository: sakaiproject/sakai
Length of output: 180
🌐 Web query:
Sakai project SocialNetworkingInfo.java userUuid userId JPA
💡 Result:
Short answer: In Profile2 the SocialNetworkingInfo class stores the user's UUID in a field named userUuid (not a numeric userId) and ProfileLogic/DAO methods often use the parameter name userId while passing the UUID string. The SocialNetworkingInfo source in Profile2 is a simple POJO (no JPA @entity annotations) with a single identifying field userUuid. [1][2]
Key references:
- SocialNetworkingInfo.java shows the field userUuid and no JPA annotations. [1]
- ProfileLogicImpl and ProfileDao use/getSocialNetworkingInfo(...) with a parameter named userId but they pass and expect the UUID string; saveSocialNetworkingInfo logs socialNetworkingInfo.getUserUuid(). [2][3]
Implication / guidance:
- If you need JPA persistence for SocialNetworkingInfo, you must convert the POJO to a JPA entity (add @entity, @table, an @id mapping — e.g. map userUuid as the primary key — and appropriate column annotations) and update the DAO implementation to use JPA EntityManager/Repository queries. Otherwise Profile2 uses its own DAO implementation and treats user identifiers as UUID strings.
Sources:
[1] SocialNetworkingInfo.java (Profile2 source).
[2] ProfileLogicImpl.getSocialNetworkingInfo / saveSocialNetworkingInfo (Profile2 logic).
[3] ProfileDao.getSocialNetworkingInfo declaration (Profile2 DAO).
🌐 Web query:
JPA @PrePersist @PreUpdate lifecycle callback field synchronization best practices
💡 Result:
Short best-practices for keeping fields (audit or derived) synchronized with @PrePersist / @PreUpdate
-
Prefer a dedicated, lightweight listener (entity listener class) or a framework solution (Spring Data JPA auditing) instead of ad‑hoc logic in many entities. These centralize behavior and reduce duplication. [1][2]
-
Keep callback methods tiny and side‑effect free: set simple scalar fields (createdAt, updatedAt, createdBy, updatedBy). Do NOT perform heavy I/O, business logic, or complex relationship manipulation in callbacks. [1][3]
-
Avoid mutating associations or collections inside lifecycle callbacks. Collection mutations may not trigger dirty detection (and can cause unexpected cascades or different instances during merge). Update only fields on the entity itself. [3][6]
-
Be careful with merge vs persist and detached instances: callbacks can behave differently (Hibernate may call listeners on different instances when merging). Test both code paths and prefer using persist for new entities when you need the same instance in callbacks. Consider using @Version to ensure updates are detected. [5][4]
-
Protect created/immutable fields: annotate creation columns as updatable = false (or set them only in @PrePersist) so updates don’t overwrite them accidentally. Populate updatedAt/updatedBy in @PreUpdate. [2][8]
-
If you need reliable, tamper‑proof timestamps, consider doing the timestamping in the database (triggers, default/ON UPDATE columns) or use application-level explicit setting (service layer) rather than relying solely on JPA callbacks. [1][4]
-
Always write unit/integration tests that exercise persist, merge, flush, and collection changes to confirm callbacks run and values are persisted as expected. Logically separate concerns (auditing vs business logic) so tests are focused. [3][4]
References
- Hibernate/JPA callback semantics (Hibernate user guide). [1]
- Hibernate ORM (6.x) callbacks docs. [9]
- Common pitfalls / Stack Overflow reports about PrePersist/PreUpdate and collection/merge issues. [5][0]
- Spring Data JPA auditing overview (how @CreatedDate/@LastModifiedDate work). [8]
- Community posts arguing tradeoffs of entity listeners vs explicit service-layer auditing. [4]
If you want, I can paste a small, testable example (BaseAuditable + entity listener or Spring Data auditing) that follows these practices.
🌐 Web query:
Sakai GitHub profile2 SocialNetworkingInfo @Entity @PrePersist @PreUpdate JPA entity
💡 Result:
Below is a minimal JPA entity example named SocialNetworkingInfo that stores a GitHub profile URL (githubProfile / profile2), with automatic createdAt/updatedAt management using @PrePersist and @PreUpdate. Adjust types, column names, validation, and package/imports to fit your project.
Code:
package com.example.model;
import jakarta.persistence.*;
import java.time.Instant;
@entity
@table(name = "social_networking_info")
public class SocialNetworkingInfo {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
// primary GitHub profile
@Column(name = "github_profile", length = 512)
private String githubProfile;
// alternative profile field (profile2)
@Column(name = "github_profile2", length = 512)
private String githubProfile2;
@Column(name = "created_at", nullable = false, updatable = false)
private Instant createdAt;
@Column(name = "updated_at", nullable = false)
private Instant updatedAt;
// constructors, getters, setters omitted for brevity
@PrePersist
protected void onCreate() {
Instant now = Instant.now();
this.createdAt = now;
this.updatedAt = now;
}
@PreUpdate
protected void onUpdate() {
this.updatedAt = Instant.now();
}
}
Notes:
- Use @PrePersist to initialize createdAt/updatedAt before the first insert and @PreUpdate to refresh updatedAt before updates. [1][2]
- If you prefer a separate listener class, annotate the entity with @EntityListeners(YourListener.class) and implement callback methods there. [1]
Sources:
[1] Baeldung — JPA Entity Lifecycle Events.
[2] Java Code Geeks — JPA Entity Lifecycle Events (examples for @PrePersist/@PreUpdate).
🌐 Web query:
site:github.com/sakaiproject/sakai profile2 SocialNetworkingInfo userId userUuid
💡 Result:
I ran that search and found no direct matches for "SocialNetworkingInfo" in the sakaiproject/sakai GitHub repo. Related results mentioning the Profile2 tool and entity providers (userUuid routes) were found:
- sakai repository root (no SocialNetworkingInfo symbol found). [1]
- A Sakai gist / log showing Profile2 entity providers and routes (mentions userUuid in URLs). [2]
If you want, I can:
- search the full Sakai history/branches for a class or file named SocialNetworkingInfo, or
- search other repos (sakaicontrib, older branches) for that symbol — which would be more exhaustive. Which would you prefer?
🌐 Web query:
JPA @PrePersist @PreUpdate synchronize two fields same value potential issues
💡 Result:
Short answer: it usually works, but there are several pitfalls to watch for when you synchronize two fields (e.g., copy valueA -> valueB) in @PrePersist/@PreUpdate.
Potential problems
- Missed callback execution: callbacks run only for managed entities at flush/commit; detached entities, no-flush tests, or unexpected transaction boundaries can make callbacks not run when you expect. [1]
- Dirty-checking / no additional update: if you change a field in @PreUpdate but the persistence provider doesn’t detect it as a new dirty state (or you mark the column as database-generated), the change may not be written. Database-generated columns annotated as generated can be reloaded instead of written. [2][3]
- Version/timestamp semantics: updating fields in callbacks can interact poorly with optimistic locking/versioning or with @UpdateTimestamp/@CreationTimestamp / Spring Data auditing (double-updates or surprising timestamps). Prefer built-in audit annotations when available. [4][6]
- Entity proxies / lazy loading: accessing lazy associations inside callbacks can trigger lazy-load exceptions (if detached) or unexpected loads during flush. Keep callbacks simple and avoid navigation that requires new DB access. [1][3]
- Ordering & listener interactions: @PrePersist/@PreUpdate ordering with @EntityListeners, inheritance, or other callbacks may produce unexpected values if multiple listeners modify the same fields. [4]
- Concurrency / race conditions: if you rely on synchronizing two columns for constraints or business rules, concurrent updates can race between read/modify/write; use DB constraints or transactions to enforce invariants.
- Tests confusion: unit/integration tests that use @transactional or don’t call flush may see different timing/behavior for @PreUpdate vs @PrePersist (e.g., @PreUpdate may run only at transaction end). [1]
- equals()/hashCode()/cached state: changing fields used in equals/hashCode or used as map keys during lifecycle can break collections or caches.
Recommendations
- Prefer provider-supported audit annotations (@CreationTimestamp, @UpdateTimestamp) or Spring Data auditing for timestamps and audit fields. [4][6]
- If you must use callbacks: keep logic simple, modify only basic fields (not associations), ensure entities are managed and flushed, and write tests that call flush() to trigger callbacks. [1][3]
- Avoid relying on callbacks for critical invariants—enforce them with DB constraints or application-layer transactional checks.
- If you see missed updates, check: field generation settings (database-generated?), transaction/flush timing, and whether the entity is detached or proxied.
Sources
[1] StackOverflow — JPA @PreUpdate not called on all entities (flush/transaction explanation).
[2] StackOverflow / blog posts — using @PrePersist/@PreUpdate vs DB-generated columns and reloading behavior.
[3] Discussions/examples showing updating timestamps in @PrePersist/@PreUpdate and alternatives.
[4] Spring Data / Hibernate docs and examples recommending @CreationTimestamp/@UpdateTimestamp and Spring Data auditing.
If you want, I can: (a) show a compact safe example for copying two fields in an entity listener, or (b) review your specific entity code and point out any problems.
🌐 Web query:
JPA entity field synchronization userUuid userId bidirectional sync database consistency
💡 Result:
Short answer — pick a single source of truth and keep the other value derived or copied only in lifecycle hooks (or let the DB enforce it). Common safe approaches:
- Prefer one canonical mapping
- Keep either the relationship (User user; @manytoone) or the FK column (UUID userUuid / Long userId), not both as independently writable fields.
- Expose the other value as a derived getter (e.g. getUserUuid() { return user == null ? userUuid : user.getUuid(); }) so there’s no risk of drift. (This is the simplest and safest.) [1][3]
- If you must store both columns in the entity
- Mark one column read-only in JPA and update it from the authoritative field: e.g. @manytoone @joincolumn(name="user_id", insertable=false, updatable=false) private User user; and a separate userId column. Populate/set the authoritative field before save. Note: JPA won’t magically populate the read-only side after insert — you may need saveAndFlush() + refresh() to see DB values. [3]
- Use lifecycle callbacks carefully
- Use @PrePersist / @PreUpdate (or an EntityListener) to copy values (userUuid = user.getUuid(); userId = user.getId()). This works for inserts/updates but watch provider quirks (merge vs persist and callback instance differences). Test with your provider/version. [2][5]
- For database-level consistency
- Enforce consistency in the database (triggers, generated columns, or foreign-key constraints). If absolute consistency is required even for out-of-JPA operations, enforce it in the DB rather than relying only on JPA callbacks. [2]
- Alternate: compute one value instead of storing it
- Use a computed column / @formula (Hibernate) or a database view if you only need the value for reads — avoids duplication and consistency problems. [2]
Minimal example (copy authoritative relation -> FK in entity listener):
- In entity:
@PrePersist @PreUpdate
private void syncFk() {
if (user != null) userId = user.getId();
if (user != null) userUuid = user.getUuid();
}
Caveats: PrePersist/PreUpdate behavior can differ with merge(), and @PrePersist may be invoked on a different instance in some cases — verify in your Hibernate/JPA version and prefer explicit set-before-persist or DB enforcement for critical consistency. [2][4][5]
Recommended practice: avoid duplicated writable columns. If duplication is unavoidable, use an authoritative field + lifecycle copy OR DB-side enforcement and test edge cases (merge, detach/attach, saveAndFlush + refresh). [1][2][3]
Sources:
[1] Hibernate user guide — JPA callbacks / events.
[2] Baeldung — JPA lifecycle callbacks and changing fields before insert/update.
[3] Stack Overflow — issues and patterns for mapping FK field + relation (insertable=false, updatable=false) and sync after save.
[4] Stack Overflow — caveats about @PrePersist/@PreUpdate instance differences.
[5] Baeldung article on changing field values before update/insert.
Based on my verification through web search and JPA best practices analysis, I've identified several critical issues with the lifecycle callback implementation. The original review comment approved this code without flagging important concerns. Let me provide the corrected review:
Lifecycle callback synchronization has critical gaps that could cause data inconsistency or corruption.
The @PrePersist and @PreUpdate annotations synchronize userUuid with userId before database operations. However, this implementation has several risks:
-
Unidirectional sync only: The callback copies
userId→userUuidbut never the reverse. IfuserUuidis modified directly via its setter (Lombok@Datagenerates public setters), a subsequent update will overwrite it withuserId, causing data loss. -
Missing null check: If
userIdis null, the callback silently setsuserUuid = null, which may violate aNOT NULLdatabase constraint. The database error would occur at flush time with no clear indication of the root cause in the callback. -
Single source of truth violated: Per JPA best practices, maintain only one writable field. Either:
- Keep only
userIdand deriveuserUuidvia getter, OR - Protect the non-authoritative field with
insertable=false, updatable=falsein@Column, OR - Add bidirectional sync + null guards if both columns must remain writable.
- Keep only
-
No integration test shown: Verify the callback actually executes with
flush()and handles edge cases (nulluserId, directuserUuidmutation, merge operations).
Required before merge:
- Add null check:
if (this.userId != null) this.userUuid = this.userId; - Document why both fields are writable and confirm this is intentional
- Add integration tests exercising persist, update, and null scenarios
- Consider protecting
userUuidfrom direct modification ifuserIdis authoritative
🤖 Prompt for AI Agents
In
profile2/api/src/main/java/org/sakaiproject/profile2/api/model/SocialNetworkingInfo.java
around lines 66-71, the @PrePersist/@PreUpdate callback unconditionally copies
userId → userUuid which risks overwriting direct userUuid changes, writing null
into userUuid, and violating single-source-of-truth JPA best practices; fix by
(1) adding a null guard so you only copy when userId != null, (2) decide and
enforce a single authoritative field—either remove write access to the derived
field (e.g., mark userUuid column insertable=false, updatable=false or remove
its setter) or implement bidirectional sync (also copy userUuid → userId when
appropriate) with clear conditions to avoid overwrite loops, (3) add
class-level/comments documenting which field is authoritative, and (4) add
integration tests for persist, update/merge, direct userUuid mutation, and null
userId scenarios before merging.
1 participant
@adrianfish please take a look
During the migration from Hibernate XML mappings to JPA annotations in SAK-51697, the
SocialNetworkingInfo entity was incorrectly updated to only map the USER_ID column.
However, the database table PROFILE_SOCIAL_INFO_T contains both USER_ID (Primary Key) and
USER_UUID (NOT NULL) columns, both of which must be populated.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.