Conversation
…ting files into multiple sessions
chris-adam
force-pushed
the
SE-270/discriminate_session_size
branch
from
1b38199 to
2f7437f
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-session size tracking and enforcement: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Utils as Utils (get_filesize, add_files_to_session)
participant Registry as Registry (get_registry_max_session_size)
participant Sessions as SessionStore
Client->>Utils: add_files_to_session(uids, signers, ...)
Utils->>Utils: sum = Σ get_filesize(uid)
Utils->>Registry: get_registry_max_session_size()
Utils->>Sessions: discriminate_sessions(signers, ..., size=sum)
Sessions-->>Utils: candidate session(s)
alt fits in an existing session
Utils->>Sessions: update session (append files, session["size"] += sum)
else create new session
Utils->>Sessions: create session with session["size"] = sum
end
Utils-->>Client: return session info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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
🤖 Fix all issues with AI agents
In `@src/imio/esign/utils.py`:
- Around line 33-38: The get_filesize function can raise AttributeError when
annex.file is None; update get_filesize (which uses uuidToObject and annex) to
guard the fallback path by checking annex.file is not None before accessing
annex.file.size and return a safe default (e.g., 0) if annex.file is missing;
keep the existing categorized_elements branch intact and only use
annex.file.size when annex.file exists.
| def get_filesize(uid): | ||
| annex = uuidToObject(uuid=uid, unrestricted=True) | ||
| if hasattr(annex.__parent__, "categorized_elements"): | ||
| return annex.__parent__.categorized_elements.get(uid, {}).get("filesize", 0) | ||
| return annex.file.size | ||
|
|
There was a problem hiding this comment.
Missing null check for annex.file in fallback path.
If annex.file is None, accessing annex.file.size on line 37 will raise an AttributeError. Other functions in this file (e.g., get_files_from_uids at line 294) explicitly check for this condition.
🛡️ Proposed fix
def get_filesize(uid):
annex = uuidToObject(uuid=uid, unrestricted=True)
if hasattr(annex.__parent__, "categorized_elements"):
return annex.__parent__.categorized_elements.get(uid, {}).get("filesize", 0)
+ if not annex.file:
+ return 0
return annex.file.size📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_filesize(uid): | |
| annex = uuidToObject(uuid=uid, unrestricted=True) | |
| if hasattr(annex.__parent__, "categorized_elements"): | |
| return annex.__parent__.categorized_elements.get(uid, {}).get("filesize", 0) | |
| return annex.file.size | |
| def get_filesize(uid): | |
| annex = uuidToObject(uuid=uid, unrestricted=True) | |
| if hasattr(annex.__parent__, "categorized_elements"): | |
| return annex.__parent__.categorized_elements.get(uid, {}).get("filesize", 0) | |
| if not annex.file: | |
| return 0 | |
| return annex.file.size |
🤖 Prompt for AI Agents
In `@src/imio/esign/utils.py` around lines 33 - 38, The get_filesize function can
raise AttributeError when annex.file is None; update get_filesize (which uses
uuidToObject and annex) to guard the fallback path by checking annex.file is not
None before accessing annex.file.size and return a safe default (e.g., 0) if
annex.file is missing; keep the existing categorized_elements branch intact and
only use annex.file.size when annex.file exists.
dc94c77 to
381238b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/imio/esign/utils.py (1)
85-102:⚠️ Potential issue | 🟠 Major
session["size"]only grows; removals don’t decrement it.This will quickly make size-based discrimination inaccurate and overly restrictive. Track per-file sizes and subtract on removal (or recompute from remaining files).
🧩 Proposed fix (track per-file size and decrement on removal)
- size = sum(get_filesize(uid) for uid in files_uids) + sizes = [get_filesize(uid) for uid in files_uids] + size = sum(sizes) @@ - for uid in files_uids: + for uid, file_size in zip(files_uids, sizes): annex = uuidToObject(uuid=uid, unrestricted=True) @@ session["files"].append( { "scan_id": annex.scan_id, "filename": new_filename + ext, "title": annex.title or "no_title", "uid": uid, "context_uid": context_uid, "status": "", + "size": file_size, } )def remove_files_from_session(files_uids): @@ - del session["files"][i] + removed = session["files"][i] + del session["files"][i] + if "size" in removed: + session["size"] = max(0, session.get("size", 0) - removed["size"])
🤖 Fix all issues with AI agents
In `@src/imio/esign/utils.py`:
- Around line 69-79: When an explicit session_id is provided the code skips
size-based discrimination so oversized batches can create or reuse sessions
beyond the cap; update the block that handles session_id/session so it checks
the computed size (from get_filesize) against the configured max session size
(use the same limit used by discriminate_sessions) and if size exceeds the max,
log an error and set session_id = session = None (or otherwise refuse to attach
to a session) so the flow treats it as no-session and prevents creating/using an
oversized session; reference get_session_annotation, get_filesize, size,
session_id, session, and discriminate_sessions to locate where to add the
validation and logging.
| annot = get_session_annotation() | ||
| size = sum(get_filesize(uid) for uid in files_uids) | ||
| if session_id is not None: | ||
| if session_id not in annot["sessions"]: | ||
| logger.error("Session with id %s not found in esign annotations.", session_id) | ||
| session_id = session = None | ||
| else: | ||
| session = annot["sessions"][session_id] | ||
| else: | ||
| session_id, session = discriminate_sessions(signers, seal, acroform, discriminators=discriminators) | ||
| session_id, session = discriminate_sessions(signers, seal, acroform, discriminators=discriminators, size=size) | ||
| if not session: |
There was a problem hiding this comment.
Max session size isn’t enforced for explicit session_id or oversized batches.
If session_id is provided, size discrimination is bypassed, and a single batch larger than the max can still create a session. This allows sessions to exceed the configured cap.
🛡️ Proposed fix
annot = get_session_annotation()
- size = sum(get_filesize(uid) for uid in files_uids)
+ size = sum(get_filesize(uid) for uid in files_uids)
+ max_size = get_registry_max_session_size() * 1024**2
+ if size > max_size:
+ logger.error("Files total size %s exceeds max session size %s.", size, max_size)
+ return None, None
if session_id is not None:
if session_id not in annot["sessions"]:
logger.error("Session with id %s not found in esign annotations.", session_id)
session_id = session = None
else:
session = annot["sessions"][session_id]
+ if session.get("size", 0) + size > max_size:
+ logger.error("Session %s would exceed max session size %s.", session_id, max_size)
+ return None, None
else:
session_id, session = discriminate_sessions(signers, seal, acroform, discriminators=discriminators, size=size)🤖 Prompt for AI Agents
In `@src/imio/esign/utils.py` around lines 69 - 79, When an explicit session_id is
provided the code skips size-based discrimination so oversized batches can
create or reuse sessions beyond the cap; update the block that handles
session_id/session so it checks the computed size (from get_filesize) against
the configured max session size (use the same limit used by
discriminate_sessions) and if size exceeds the max, log an error and set
session_id = session = None (or otherwise refuse to attach to a session) so the
flow treats it as no-session and prevents creating/using an oversized session;
reference get_session_annotation, get_filesize, size, session_id, session, and
discriminate_sessions to locate where to add the validation and logging.
There was a problem hiding this comment.
Yes, providing a session ID by pass any discriminator
There was a problem hiding this comment.
@chris-adam, Thank you for clarifying! I understand that bypassing discriminators when an explicit session_id is provided is intentional behavior. This gives callers direct control to add files to a specific session regardless of size limits or other discriminator criteria.
If this is the desired behavior, the implementation is correct as-is. The size limit effectively applies only during automatic session discrimination (when session_id is None), not when explicitly targeting a session.
✏️ Learnings added
Learnt from: chris-adam
Repo: IMIO/imio.esign PR: 8
File: src/imio/esign/utils.py:69-79
Timestamp: 2026-02-09T08:37:06.190Z
Learning: In `src/imio/esign/utils.py`, when an explicit `session_id` is provided to `add_files_to_session()`, all discriminators (including size limits) are intentionally bypassed. Size-based discrimination only applies during automatic session selection when `session_id` is `None`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
sgeulette
left a comment
sgeulette
left a comment
There was a problem hiding this comment.
ok pour la logique.
Peux-tu ajouter/modifier les tests ?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/imio/esign/tests/test_utils.py`:
- Line 6: The import line referencing IMIO_ESIGN_INTEGRATION_TESTING contains an
unnecessary noqa directive; remove the trailing "# noqa: E501" from the import
(the symbol to edit is IMIO_ESIGN_INTEGRATION_TESTING in the import statement)
so the line is just a normal import without the unused noqa comment.
- Around line 250-276: The test test_session_size_discrimination mutates global
state by calling set_registry_max_session_size(1) without restoring it; capture
the previous value before calling set_registry_max_session_size (e.g. prev =
set_registry_max_session_size(...)? or call a getter if available), then restore
it in a teardown/cleanup or a finally block so other tests aren’t affected;
update the test to use addCleanup or a try/finally around the call to
set_registry_max_session_size to ensure the original registry value is restored
after the test finishes, referencing set_registry_max_session_size and
test_session_size_discrimination to locate the change.
sgeulette
left a comment
sgeulette
left a comment
There was a problem hiding this comment.
j'ai corrigé et amélioré un test
2 participants
Summary by CodeRabbit
New Features
Tests