Skip to content

[MS-951]: Feature/add room module#1161

Merged
meladRaouf merged 11 commits into
mainfrom
feature/add-room-module
Jun 9, 2025
Merged

[MS-951]: Feature/add room module#1161
meladRaouf merged 11 commits into
mainfrom
feature/add-room-module

Conversation

@meladRaouf
Copy link
Copy Markdown
Collaborator

@meladRaouf meladRaouf commented Apr 10, 2025

JIRA ticket
Will be released in: 2025.2.0

Notable changes

The changes include the definition of database entities (DbSubject, DbBiometricTemplate), Data Access Objects (SubjectDao), and the local data source implementation (RoomEnrolmentRecordLocalDataSource) to manage subject and biometric data.

  • Database Entities:
    • Added DbSubject entity
    • Added DbBiometricTemplate entity to store individual biometric samples (face, fingerprints) linked to a DbSubject.
  • SubjectDao (Data Access Object):
    • Created SubjectDao interface with methods for:
      • Inserting, updating, and deleting DbSubject, DbBiometricTemplate records.
      • Querying subjects (getSubject, loadSubjects via RawQuery).
      • Counting subjects with various filter combinations (countSubjects for specific formats or general counts).
      • Loading biometric samples (loadSamples) with filtering (format, project, subject, attendant, module) and pagination (limit, offset), returning a map of subject IDs to their templates.
  • RoomEnrolmentRecordLocalDataSource:
    • Implemented the local data source using SubjectDao to interact with the Room database.
    • Custom SQL building was used in the data source instead of relying solely on DAO queries to gain more control over dynamic query generation, especially for complex filtering scenarios. This approach avoids performance pitfalls commonly introduced by OR conditions in Room queries—conditions that make indexing ineffective. For example, a clause like (:projectId IS NULL OR s.projectId = :projectId) disables the use of an index on projectId, because the database must evaluate both sides of the OR for every row. By building SQL manually, we include only the active filters (e.g., s.projectId = :projectId when projectId is not null), which ensures the query remains index-friendly. This approach leads to more efficient execution plans and significantly better performance on large datasets.

Testing guidance

  • Test all DB operations Add, delete and update.
  • All DB retrieval scenarios count subjects and list face or fingerprint templates using verify or identify flows

Additional work checklist

  • Effect on other features and security has been considered
  • Design document marked as "In development" (if applicable)
  • External (Gitbook) and internal (Confluence) Documentation is up-to-date (or ticket created)
  • Test cases in Testiny are up to date (or ticket created)
  • Other teams notified about the changes (if applicable)

@cla-bot cla-bot Bot added the ... label Apr 10, 2025
@meladRaouf meladRaouf force-pushed the feature/add-room-module branch 2 times, most recently from 40d5442 to 9830fbd Compare April 10, 2025 11:39
@meladRaouf meladRaouf force-pushed the feature/add-room-module branch 11 times, most recently from 25aa5d9 to e56bd19 Compare May 6, 2025 21:17
@meladRaouf meladRaouf requested review from a team, BurningAXE, TristramN, alex-vt, alexandr-simprints, luhmirin-s and ybourgery and removed request for a team May 7, 2025 05:26
@meladRaouf meladRaouf marked this pull request as ready for review May 7, 2025 05:26
Copy link
Copy Markdown
Contributor

@luhmirin-s luhmirin-s left a comment

Choose a reason for hiding this comment

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

Amazing job! I must admit that I just skimmed on THE test file :D

I have couple of small to mid-size concerns in the comments, the general approach LGTM.

suspend fun getSubject(subjectId: String): SubjectBiometrics?

@RawQuery
suspend fun loadSubjects(query: SupportSQLiteQuery): List<SubjectBiometrics>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is a bit concerning that query building is disjointed from the returning model definition; sooner or later, a bug will be caused by changing one place and forgetting another.

I am not sure what would be the best way to fix it, tho.

Copy link
Copy Markdown
Contributor

@BurningAXE BurningAXE left a comment

Choose a reason for hiding this comment

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

Great job! 🥇

@meladRaouf meladRaouf force-pushed the feature/add-room-module branch 3 times, most recently from b8a4726 to 74baf3c Compare June 2, 2025 08:08
@meladRaouf meladRaouf force-pushed the feature/add-room-module branch 3 times, most recently from 41f8f23 to 0d156b9 Compare June 4, 2025 13:16
): List<MatchResult> {
val simAfisCandidates = candidates.map { it.toSimAfisPerson() }

println("Matching ${simAfisCandidates.size} candidates using all ${jniLibAfis.getNbCores()} cores")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why did you delete this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't like using the println, but I reverted it back

onCandidateLoaded = {
loadedCandidates.incrementAndGet()
this.trySend(MatcherState.CandidateLoaded)
runBlocking {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really need this to be blocking?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Found and implemented a better solution to suspend the reader

.query(caseDataUri, null, null, null, null)
?.use { caseDataCursor ->
val subjectActions = getSubjectActionsValue(caseDataCursor)
Simber.d(subjectActions)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is often useful

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reverted back

@meladRaouf meladRaouf force-pushed the feature/add-room-module branch from 5d3413d to a3a162c Compare June 9, 2025 03:52
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 9, 2025

@meladRaouf meladRaouf merged commit 02a49a8 into main Jun 9, 2025
12 checks passed
@meladRaouf meladRaouf deleted the feature/add-room-module branch June 9, 2025 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants