Ginger OCR Engine Change#4304
Conversation
WalkthroughReplaced Tesseract with PaddleOCR (Sdcb.PaddleOCR) across OCR operations, switched image IO to OpenCvSharp Mats, added PdfiumViewer-based PDF rendering and PdfPig table extraction, removed embedded tessdata assets from the project, and added OpenCV/Pdfium/PaddleOCR NuGet dependencies. (≈36 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant G as GingerOcrOperations
participant P as PdfiumViewer
participant CV as OpenCvSharp (Mat)
participant O as PaddleOCR (PaddleOcrAll)
Caller->>G: ReadTextFromPdfAllPages(filePath, dpi, pages)
G->>P: Open PDF & render requested pages -> PNG byte[] %%#ADD8E6
loop each page
G->>CV: Convert PNG byte[] -> Mat
G->>O: Instance.Run(Mat) %%#90EE90
O-->>G: OCR Result (regions, text)
G-->>G: Aggregate region texts into Page_N
end
G-->>Caller: Dictionary{Page_1: text, Page_2: text, ...}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCore/GingerCore.csproj(1 hunks)Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T08:48:11.915Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCore/GingerCore.csproj:538-538
Timestamp: 2025-08-25T08:48:11.915Z
Learning: In the Ginger codebase, the OWASPZAPDotNetAPI package reference in GingerCore.csproj is required for architectural reasons, even when direct usage is not immediately visible in the codebase. This was confirmed by AmanPrasad43 during the security testing feature implementation.
Applied to files:
Ginger/GingerCore/GingerCore.csproj
🧬 Code graph analysis (1)
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (1)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
🔇 Additional comments (4)
Ginger/GingerCore/GingerCore.csproj (2)
538-541: OpenCV package additions look correct.The chosen OpenCvSharp4 versions are consistent and include the Windows runtime. No issues spotted.
538-556: Tesseract package & tessdata still present — confirm and remove if unused
- Evidence: Ginger/GingerCore/GingerCore.csproj contains a PackageReference to Tesseract (Version 5.2.0) and ItemGroup entries copying tessdata (tessdata\eng.traineddata and tessdata\pdf.ttf); the file Ginger/GingerCore/tessdata/eng.traineddata exists in the repo.
- Action: If PaddleOCR fully replaces Tesseract, remove the PackageReference and the tessdata resource/None entries and delete the tessdata folder to reduce output size; otherwise identify which components still depend on Tesseract.
- Verify: Search the repo for usages before removal (e.g., "Tesseract", "TesseractEngine", "TessBaseAPI", "tessdata") and confirm no runtime dependencies remain.
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (2)
42-60: Cannot verify — repo search returned no files; manual verification requiredRepository files were not found during the search, so I couldn't confirm call sites. Instance now returns PaddleOcrAll (previously TesseractEngine) — ensure no callers of GingerOcrOperations.Instance rely on TesseractEngine APIs. From the repository root run: rg -n --hidden -S '\bGingerOcrOperations\s*.\s*Instance\b' && rg -n --hidden -S '\bTesseractEngine\b' to find and update callers as needed.
40-56: Singleton + global lock will serialize all OCR; consider bounded parallelism and disposal.
- The static lock around Instance and every Run call forces single‑threaded OCR, potentially throttling scenarios that process many images/PDF pages.
- PaddleOcrAll likely implements IDisposable; add a way to dispose/reset models on shutdown or when switching languages.
Suggested change (conceptual):
- Replace lock with SemaphoreSlim to allow N concurrent runs.
- Add a Reset/Dispose method.
Confirm whether PaddleOcrAll is thread‑safe across Run; if yes, remove per‑call locking. If not, gate concurrency with a SemaphoreSlim(Env var/configurable).
| <PackageReference Include="PdfiumViewer" Version="2.13.0" /> | ||
| <PackageReference Include="PdfiumViewer.Native.x86_64.v8-xfa" Version="2018.4.8.256" /> |
There was a problem hiding this comment.
🧩 Analysis chain
Pdfium native package is x64-only while the project targets AnyCPU; also the native build is very old.
- Runtime risk: PdfiumViewer.Native.x86_64.v8-xfa forces x64. With PlatformTarget=AnyCPU, a 32‑bit process will fail to load the native PDFium DLLs.
- Security/maintenance: The 2018 native build is significantly outdated; verify for known CVEs and consider a newer native packaging strategy.
Action options:
- Prefer x64 everywhere:
- <PlatformTarget>AnyCPU</PlatformTarget>
+ <PlatformTarget>x64</PlatformTarget>- Or keep AnyCPU and add the x86 native package alongside the x64 one (if 32‑bit must be supported), and/or move to a modern Pdfium packaging.
Please confirm whether Ginger runs exclusively as x64. If not, add the x86 native package variant or force x64. Also, check for advisories and newer Pdfium builds:
🌐 Web query:
What is the latest stable version of PdfiumViewer on NuGet, and what native packages are recommended for x64 and x86? Are there known vulnerabilities for Pdfium builds circa 2018.4.8.256?
💡 Result:
-
Latest stable “official” PdfiumViewer on NuGet: PdfiumViewer 2.13.0. [1]
(There are maintained forks/variants — e.g. PdfiumViewer.Updated 2.14.5 and PdfiumViewer.Net.WPF / other packages — if you need .NET Core/.NET 6+ support.) [2] -
Recommended native PDFium packages (used by many PdfiumViewer builds): PdfiumViewer.Native.x86.v8-xfa (for x86) and PdfiumViewer.Native.x86_64.v8-xfa (for x64). Many PdfiumViewer/WPF packages reference the 2018.4.8.256 native build; alternative no_v8/no_xfa variants also exist. [3][4]
-
Known vulnerabilities for Pdfium builds around 2018.4.8.256: yes — PDFium had several security issues in 2018 (memory-corruption / use-after-free / integer overflow / out‑of‑bounds reads) that were assigned CVEs (examples: CVE-2018-6088, CVE-2018-6072, CVE-2018-6031, CVE-2018-17469, CVE-2018-16076, etc.). If you must use a 2018-era binary, treat it as vulnerable and either update to patched PDFium binaries or use a modern maintained fork that ships updated PDFium builds. [5][6][7][9][8]
Sources: NuGet package pages and native-package listings; NVD/CVE and vendor advisories for PDFium CVEs. [1][2][3][4][5][6][7][8][9]
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "=== Show lines ~520-560 of Ginger/GingerCore/GingerCore.csproj ==="
if [ -f Ginger/GingerCore/GingerCore.csproj ]; then
nl -ba Ginger/GingerCore/GingerCore.csproj | sed -n '520,560p'
else
echo "File not found: Ginger/GingerCore/GingerCore.csproj"
fi
echo
echo "=== Search for PlatformTarget and Prefer32Bit ==="
rg -n --hidden -S "<PlatformTarget>|<Prefer32Bit>" || true
echo
echo "=== Search for AnyCPU / 'Any CPU' occurrences ==="
rg -n --hidden -S "AnyCPU|Any CPU" || true
echo
echo "=== Search for RuntimeIdentifier(s) ==="
rg -n --hidden -S "RuntimeIdentifier|RuntimeIdentifiers|win-x64|win-x86|win-x86_64" || true
echo
echo "=== Search for PdfiumViewer and native package references ==="
rg -n --hidden -S "PdfiumViewer" || true
echo
echo "=== Check for Directory.Build.props/targets that might set PlatformTarget globally ==="
rg -n --hidden -S "Directory.Build.props|Directory.Build.targets" || true
echo
echo "=== Search solution (.sln) files for 'Any CPU' or platform mappings ==="
rg -n --hidden -S "Any CPU|AnyCPU" --glob '!**/node_modules/**' || trueLength of output: 132
🏁 Script executed:
#!/bin/bash
# Safer verification run — avoid failing early if some tools are missing.
echo "PWD: $(pwd)"
echo
CSProj="Ginger/GingerCore/GingerCore.csproj"
if [ -f "$CSProj" ]; then
echo "=== Show first 800 lines of $CSProj with line numbers ==="
if command -v awk >/dev/null 2>&1; then
awk '{printf "%6d: %s\n", NR, $0}' "$CSProj" | sed -n '1,800p'
else
sed -n '1,800p' "$CSProj"
fi
else
echo "File not found: $CSProj"
fi
GREP="grep -RIn --binary-files=without-match --exclude-dir=.git --exclude-dir=node_modules --exclude='*.dll' --exclude='*.png'"
echo
echo "=== Search for PlatformTarget and Prefer32Bit ==="
$GREP -e "<PlatformTarget>" -e "<Prefer32Bit>" || true
echo
echo "=== Search for AnyCPU / 'Any CPU' occurrences ==="
$GREP -e "AnyCPU" -e "Any CPU" || true
echo
echo "=== Search for RuntimeIdentifier(s) (win-x64 / win-x86 / RID) ==="
$GREP -e "RuntimeIdentifier" -e "RuntimeIdentifiers" -e "win-x64" -e "win-x86" || true
echo
echo "=== Search for PdfiumViewer and native package references ==="
$GREP -e "PdfiumViewer" || true
echo
echo "=== Check for Directory.Build.props/targets that might set PlatformTarget globally ==="
if [ -f "Directory.Build.props" ]; then
echo "Found Directory.Build.props (first 200 lines):"
if command -v awk >/dev/null 2>&1; then
awk '{printf "%6d: %s\n", NR, $0}' Directory.Build.props | sed -n '1,200p'
else
sed -n '1,200p' Directory.Build.props
fi
else
$GREP -e "Directory.Build.props" || true
fi
echo
echo "=== Search solution (.sln) files for 'Any CPU' or platform mappings ==="
$GREP -e "Any CPU" -e "AnyCPU" --include='*.sln' || trueLength of output: 80153
Fix PDFium native packaging and update the 2018 PDFium binary (critical)
- Evidence: Ginger/GingerCore/GingerCore.csproj has AnyCPU (line 41) but references PdfiumViewer.Native.x86_64.v8-xfa Version="2018.4.8.256" (lines 542–543). Installer embeds win-x64 runtimes (Ginger/GingerInstallerScript.iss lines ~51–52).
- Risk: the native package is x64-only — AnyCPU running as 32-bit will fail to load PDFium; the 2018 PDFium build is known to have multiple 2018-era CVEs (memory corruption / UAF / OOB).
- Fix (pick one):
- Force x64 for affected projects (example diff):
- <PlatformTarget>AnyCPU</PlatformTarget> + <PlatformTarget>x64</PlatformTarget>
- OR keep AnyCPU and add the x86 native package (PdfiumViewer.Native.x86.v8-xfa) alongside the x64 package and ensure correct runtime native resolution.
- In all cases replace the 2018.4.8.256 native package with patched/newer PDFium binaries or switch to a maintained Pdfium packaging that ships up-to-date native builds.
- Force x64 for affected projects (example diff):
- Action: apply chosen fix, update native package, and verify install/run on target platforms.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
Ginger/GingerCore/GingerCore.csproj lines 542-543: the project is AnyCPU (line
~41) but only references the x64-only PdfiumViewer.Native.x86_64.v8-xfa
v2018.4.8.256 which can fail when the process runs 32-bit and also contains old
vulnerable PDFium binaries; fix by either 1) forcing x64 for this project and
any installers/packaging so the x64 native package is correct, or 2) keep AnyCPU
and add the matching x86 native package (PdfiumViewer.Native.x86.v8-xfa) and
ensure the runtime native resolution loads the correct arch at runtime; in
either case replace the 2018.4.8.256 native package with a patched/newer Pdfium
native package (or switch to a maintained Pdfium native distribution) and update
installer/runtime packaging and tests to verify both install and runtime
behavior on target platforms.
| public static string ReadTextFromPdfSinglePage(string pdfFilePath, string pageNum, int dpi, string password = null) | ||
| { | ||
| StringBuilder result = new StringBuilder(); | ||
| try | ||
| { | ||
| List<byte[]> lstByteArray = GetListOfPngByteArrayFromPdf(pdfFilePath, dpi, password); | ||
| foreach (byte[] byteArray in lstByteArray) | ||
| var pages = GetPngByteArrayFromPdf(pdfFilePath, pageNum, dpi, password); | ||
| foreach (var pageBytes in pages) | ||
| { | ||
| resultTxt = GetResultTextFromByteArray(label, resultTxt, byteArray); | ||
| if (!string.IsNullOrEmpty(resultTxt)) | ||
| using (Mat src = Cv2.ImDecode(pageBytes, ImreadModes.Color)) | ||
| { | ||
| break; | ||
| var ocrResult = SafeRun(src); | ||
| result.AppendLine(string.Join(Environment.NewLine, ocrResult.Regions.Select(r => r.Text))); | ||
| } | ||
| } | ||
| } | ||
| return resultTxt; | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); | ||
| } | ||
| return result.ToString(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Method name mismatch: supports multiple pages.
ReadTextFromPdfSinglePage accepts comma‑separated pages. Rename or document behavior to avoid confusion.
🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs around lines 284-304, the
method ReadTextFromPdfSinglePage actually accepts a comma-separated list of
pages, which is misleading; update it to reflect its real behavior by either
renaming the method to ReadTextFromPdfPages (or ReadTextFromPdfMultiplePages)
and renaming the parameter pageNum to pagesCsv (or pages) OR keep the name but
add clear XML documentation and parameter comments stating it accepts
comma-separated pages; if you rename the method update all call sites and add an
[Obsolete] shim for the old name if backward compatibility is required, and
ensure unit tests and usages are adjusted to the new name/parameter to avoid
breaking callers.
| public static Dictionary<string, object> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null) | ||
| { | ||
| Dictionary<string, object> dctOutput = []; | ||
|
|
||
| var result = new Dictionary<string, object>(); | ||
| try | ||
| { | ||
| byte[] pdfByteArray = File.ReadAllBytes(pdfFilePath); | ||
| List<byte[]> pngArray = Pdf2Png.ConvertAllPages(pdfByteArray, dpi, password); | ||
| for (int i = 0; i < pngArray.Count; i++) | ||
| var pages = GetListOfPngByteArrayFromPdf(pdfFilePath, dpi, password); | ||
|
|
||
| for (int i = 0; i < pages.Count; i++) | ||
| { | ||
| string output = ReadTextFromByteArray(pngArray[i]); | ||
| dctOutput.Add("Page No. " + i, output); | ||
| using (Mat src = Cv2.ImDecode(pages[i], ImreadModes.Color)) | ||
| { | ||
| var ocrResult = SafeRun(src); | ||
| result[$"Page_{i + 1}"] = string.Join(Environment.NewLine, ocrResult.Regions.Select(r => r.Text)); | ||
| } | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); | ||
| } | ||
| return dctOutput; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a strongly‑typed dictionary.
Values are strings; prefer Dictionary<string, string>.
- public static Dictionary<string, object> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null)
+ public static Dictionary<string, string> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null)
...
- var result = new Dictionary<string, object>();
+ var result = new Dictionary<string, string>();📝 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.
| public static Dictionary<string, object> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null) | |
| { | |
| Dictionary<string, object> dctOutput = []; | |
| var result = new Dictionary<string, object>(); | |
| try | |
| { | |
| byte[] pdfByteArray = File.ReadAllBytes(pdfFilePath); | |
| List<byte[]> pngArray = Pdf2Png.ConvertAllPages(pdfByteArray, dpi, password); | |
| for (int i = 0; i < pngArray.Count; i++) | |
| var pages = GetListOfPngByteArrayFromPdf(pdfFilePath, dpi, password); | |
| for (int i = 0; i < pages.Count; i++) | |
| { | |
| string output = ReadTextFromByteArray(pngArray[i]); | |
| dctOutput.Add("Page No. " + i, output); | |
| using (Mat src = Cv2.ImDecode(pages[i], ImreadModes.Color)) | |
| { | |
| var ocrResult = SafeRun(src); | |
| result[$"Page_{i + 1}"] = string.Join(Environment.NewLine, ocrResult.Regions.Select(r => r.Text)); | |
| } | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); | |
| } | |
| return dctOutput; | |
| return result; | |
| } | |
| public static Dictionary<string, string> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null) | |
| { | |
| var result = new Dictionary<string, string>(); | |
| try | |
| { | |
| var pages = GetListOfPngByteArrayFromPdf(pdfFilePath, dpi, password); | |
| for (int i = 0; i < pages.Count; i++) | |
| { | |
| using (Mat src = Cv2.ImDecode(pages[i], ImreadModes.Color)) | |
| { | |
| var ocrResult = SafeRun(src); | |
| result[$"Page_{i + 1}"] = string.Join(Environment.NewLine, ocrResult.Regions.Select(r => r.Text)); | |
| } | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs around lines 403 to 424,
the method currently returns Dictionary<string, object> but all values are
strings; change the method signature and local variable to use
Dictionary<string, string>, update the instantiation to new Dictionary<string,
string>(), and keep assignments like result[$"Page_{i + 1}"] = string.Join(...)
as strings; also update the method return type and any callers that expect
Dictionary<string, object> to accept Dictionary<string, string> (or convert
where needed).
| // ---------------- Table extraction (with Tabula) ---------------- | ||
| public static string ReadTextFromPdfTable( | ||
| string pdfFilePath, | ||
| string columnName, | ||
| string pageNumber, | ||
| bool useRowNumber, | ||
| int rowNumber, | ||
| ActOcr.eTableElementRunColOperator elementLocateBy, | ||
| string conditionColumnName, | ||
| string conditionColumnValue, | ||
| string password = null) | ||
| { | ||
| string txtOutput = string.Empty; | ||
| try | ||
| { | ||
| using (PdfDocument document = PdfDocument.Open(pdfFilePath, new ParsingOptions() { ClipPaths = true, Password = password })) | ||
| using (PdfPigDocument document = PdfPigDocument.Open(pdfFilePath)) | ||
| { | ||
| ObjectExtractor oe = new ObjectExtractor(document); | ||
| if (!string.IsNullOrEmpty(pageNumber)) | ||
| { | ||
| List<string> lstPageNum = GetListOfPageNos(pageNumber); | ||
| foreach (string pageNum in lstPageNum) | ||
| { | ||
| PageArea page = oe.Extract(int.Parse(pageNum)); | ||
| int pageIndex = string.IsNullOrEmpty(pageNumber) ? 1 : int.Parse(pageNumber); | ||
| Page page = document.GetPage(pageIndex); | ||
|
|
||
| // detect canditate table zones | ||
| GetTableDataFromPageArea(columnName, useRowNumber, rowNumber, conditionColumnName, conditionColumnValue, ref txtOutput, page, elementLocateBy); | ||
| if (!string.IsNullOrEmpty(txtOutput)) | ||
| { | ||
| return txtOutput; | ||
| } | ||
| } | ||
|
|
||
| } | ||
| else | ||
| { | ||
| PageIterator pgIterator = oe.Extract(); | ||
| using (pgIterator) | ||
| { | ||
| pgIterator.MoveNext(); | ||
| while (pgIterator.Current != null) | ||
| { | ||
| PageArea pgArea = pgIterator.Current; | ||
| GetTableDataFromPageArea(columnName, useRowNumber, rowNumber, conditionColumnName, conditionColumnValue, ref txtOutput, pgArea, elementLocateBy); | ||
| if (!string.IsNullOrEmpty(txtOutput)) | ||
| { | ||
| return txtOutput; | ||
| } | ||
| if (!pgIterator.MoveNext()) | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); | ||
| } | ||
| // Extract words with bounding boxes | ||
| var words = page.GetWords(); | ||
|
|
||
| Reporter.ToLog(eLogLevel.ERROR, "Unable to find text in tables", null); | ||
| return txtOutput; | ||
| } | ||
| // Group words by line (using Y coordinate) | ||
| var rows = words | ||
| .GroupBy(w => Math.Round(w.BoundingBox.Bottom, 1)) // group by Y | ||
| .OrderByDescending(g => g.Key) // top to bottom | ||
| .ToList(); | ||
|
|
||
| private static void GetTableDataFromPageArea(string columnName, bool useRowNumber, int rowNumber, string conditionColumnName, string conditionColumnValue, ref string txtOutput, PageArea page, ActOcr.eTableElementRunColOperator elementLocateBy) | ||
| { | ||
| SpreadsheetDetectionAlgorithm detector = new SpreadsheetDetectionAlgorithm(); | ||
| // Convert to row -> list of words (sorted left to right) | ||
| var tableRows = rows.Select(r => r.OrderBy(w => w.BoundingBox.Left).ToList()).ToList(); | ||
|
|
||
| IExtractionAlgorithm ea = new SpreadsheetExtractionAlgorithm(); | ||
| // Try to locate column index by columnName | ||
| int colIndex = -1; | ||
| if (!string.IsNullOrEmpty(columnName) && tableRows.Count > 0) | ||
| { | ||
| var headerRow = tableRows.First(); | ||
| colIndex = headerRow.FindIndex(w => w.Text.Equals(columnName, StringComparison.OrdinalIgnoreCase)); | ||
| } | ||
|
|
||
| List<Table> tables = ea.Extract(page); | ||
| foreach (Table table in tables) | ||
| { | ||
| if (useRowNumber) | ||
| { | ||
| IReadOnlyList<IReadOnlyList<Cell>> rows = table.Rows; | ||
| for (int i = 0; i < rows[0].Count; i++) | ||
| if (colIndex == -1) | ||
| { | ||
| Cell cellObj = rows[0][i]; | ||
| if (cellObj.GetText(false).Equals(columnName)) | ||
| { | ||
| txtOutput = rows[rowNumber + 1][i].GetText(false); | ||
| return; | ||
| } | ||
| Reporter.ToLog(eLogLevel.WARN, $"Column '{columnName}' not found in PDF table."); | ||
| return string.Empty; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| IReadOnlyList<IReadOnlyList<Cell>> rows = table.Rows; | ||
|
|
||
| int columnNameIndex = -1; | ||
| int i, j; | ||
| for (i = 0; i < rows[0].Count; i++) | ||
| if (useRowNumber) | ||
| { | ||
| Cell cellObj = rows[0][i]; | ||
| if (cellObj.GetText(false).Equals(columnName)) | ||
| if (rowNumber < tableRows.Count) | ||
| { | ||
| columnNameIndex = i; | ||
| break; | ||
| var targetRow = tableRows[rowNumber]; | ||
| if (colIndex < targetRow.Count) | ||
| return targetRow[colIndex].Text; | ||
| } | ||
| } | ||
| for (i = 0; i < rows.Count; i++) | ||
| else if (!string.IsNullOrEmpty(conditionColumnName) && !string.IsNullOrEmpty(conditionColumnValue)) | ||
| { | ||
| bool bIsConditionValFound = false; | ||
| for (j = 0; j < rows[i].Count; j++) | ||
| // Locate condition column index | ||
| int condIndex = tableRows.First().FindIndex(w => w.Text.Equals(conditionColumnName, StringComparison.OrdinalIgnoreCase)); | ||
|
|
||
| if (condIndex != -1) | ||
| { | ||
| Cell cellObj = rows[i][j]; | ||
| if (rows[0][j].GetText(false).Equals(conditionColumnName)) | ||
| foreach (var row in tableRows.Skip(1)) | ||
| { | ||
| switch (elementLocateBy) | ||
| if (condIndex < row.Count && row[condIndex].Text.Contains(conditionColumnValue)) | ||
| { | ||
| case ActOcr.eTableElementRunColOperator.Equals: | ||
| if (cellObj.GetText(false).Equals(conditionColumnValue)) | ||
| { | ||
| bIsConditionValFound = true; | ||
| break; | ||
| } | ||
| break; | ||
| case ActOcr.eTableElementRunColOperator.NotEquals: | ||
| if (!cellObj.GetText(false).Equals(conditionColumnValue)) | ||
| { | ||
| bIsConditionValFound = true; | ||
| break; | ||
| } | ||
| break; | ||
| case ActOcr.eTableElementRunColOperator.Contains: | ||
| if (cellObj.GetText(false).Contains(conditionColumnValue)) | ||
| { | ||
| bIsConditionValFound = true; | ||
| break; | ||
| } | ||
| break; | ||
| case ActOcr.eTableElementRunColOperator.NotContains: | ||
| if (!cellObj.GetText(false).Contains(conditionColumnValue)) | ||
| { | ||
| bIsConditionValFound = true; | ||
| break; | ||
| } | ||
| break; | ||
| case ActOcr.eTableElementRunColOperator.StartsWith: | ||
| if (cellObj.GetText(false).StartsWith(conditionColumnValue)) | ||
| { | ||
| bIsConditionValFound = true; | ||
| break; | ||
| } | ||
| break; | ||
| case ActOcr.eTableElementRunColOperator.NotStartsWith: | ||
| if (!cellObj.GetText(false).StartsWith(conditionColumnValue)) | ||
| { | ||
| bIsConditionValFound = true; | ||
| break; | ||
| } | ||
| break; | ||
| case ActOcr.eTableElementRunColOperator.EndsWith: | ||
| if (cellObj.GetText(false).EndsWith(conditionColumnValue)) | ||
| { | ||
| bIsConditionValFound = true; | ||
| break; | ||
| } | ||
| break; | ||
| case ActOcr.eTableElementRunColOperator.NotEndsWith: | ||
| if (!cellObj.GetText(false).Equals(conditionColumnValue)) | ||
| { | ||
| bIsConditionValFound = true; | ||
| break; | ||
| } | ||
| break; | ||
| default: | ||
| //do nothing | ||
| break; | ||
| if (colIndex < row.Count) | ||
| return row[colIndex].Text; | ||
| } | ||
| } | ||
| if (bIsConditionValFound) | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| if (bIsConditionValFound) | ||
| { | ||
| txtOutput = rows[i][columnNameIndex].GetText(false); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| return string.Empty; | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Error reading table from PDF: {ex.Message}", ex); | ||
| return string.Empty; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
PDF table extraction: password unused, elementLocateBy unused, potential row indexing off‑by‑one.
- Password parameter is ignored. PdfPig supports opening with ParsingOptions.Password.
- elementLocateBy (equals/contains) is unused; detection uses Contains only.
- Row numbering likely 1‑based in UX; code uses zero‑based without validation.
Proposed fixes:
- using (PdfPigDocument document = PdfPigDocument.Open(pdfFilePath))
+ var opts = string.IsNullOrEmpty(password) ? null : new UglyToad.PdfPig.ParsingOptions() { Password = password };
+ using (PdfPigDocument document = opts == null ? PdfPigDocument.Open(pdfFilePath) : PdfPigDocument.Open(pdfFilePath, opts))
{
int pageIndex = string.IsNullOrEmpty(pageNumber) ? 1 : int.Parse(pageNumber);
Page page = document.GetPage(pageIndex);
...
- if (useRowNumber)
+ if (useRowNumber)
{
- if (rowNumber < tableRows.Count)
+ // treat rowNumber as 1-based for data rows (excluding header)
+ int dataRowIdx = Math.Max(1, rowNumber);
+ if (dataRowIdx < tableRows.Count)
{
- var targetRow = tableRows[rowNumber];
+ var targetRow = tableRows[dataRowIdx];
if (colIndex < targetRow.Count)
return targetRow[colIndex].Text;
}
}
- else if (!string.IsNullOrEmpty(conditionColumnName) && !string.IsNullOrEmpty(conditionColumnValue))
+ else if (!string.IsNullOrEmpty(conditionColumnName) && !string.IsNullOrEmpty(conditionColumnValue))
{
// Locate condition column index
int condIndex = tableRows.First().FindIndex(w => w.Text.Equals(conditionColumnName, StringComparison.OrdinalIgnoreCase));
if (condIndex != -1)
{
foreach (var row in tableRows.Skip(1))
{
- if (condIndex < row.Count && row[condIndex].Text.Contains(conditionColumnValue))
+ if (condIndex < row.Count)
+ {
+ bool match = elementLocateBy == ActOcr.eTableElementRunColOperator.Equals
+ ? row[condIndex].Text.Equals(conditionColumnValue, StringComparison.OrdinalIgnoreCase)
+ : row[condIndex].Text.Contains(conditionColumnValue, StringComparison.OrdinalIgnoreCase);
+ if (match && colIndex < row.Count)
+ return row[colIndex].Text;
+ }
- {
- if (colIndex < row.Count)
- return row[colIndex].Text;
- }Please confirm PdfPig’s ParsingOptions.Password API availability for the version in use. If different, I can adjust the patch.
🌐 Web query:
Does UglyToad.PdfPig support opening a password-protected PDF via ParsingOptions.Password, and what is the exact Open signature?
💡 Result:
Yes. PdfPig supports opening password-protected PDFs via ParsingOptions (you can provide Password or a list Passwords). The PdfDocument.Open overloads are:
- PdfDocument Open(string filePath, ParsingOptions options = null)
- PdfDocument Open(Stream stream, ParsingOptions options = null)
- PdfDocument Open(byte[] fileBytes, ParsingOptions options = null).
(Provide the password(s) via a ParsingOptions instance, e.g. new ParsingOptions { Password = "pwd" } or { Passwords = new List{ "one","two" } }). [1][2]
Sources:
[1] PdfPig official site (examples & ParsingOptions).
[2] UglyToad.PdfPig Wiki — PdfDocument (Open overloads & ParsingOptions).
PDF table extraction: password unused, elementLocateBy unused, potential row indexing off‑by‑one.
- Password: Verified — UglyToad.PdfPig supports ParsingOptions.Password and an Open overload that accepts ParsingOptions (PdfDocument.Open(string, ParsingOptions)). Pass new UglyToad.PdfPig.ParsingOptions { Password = password } (or Passwords list) to the Open call when password is provided.
- elementLocateBy: Currently ignored — implement Equals vs Contains when matching conditionColumnValue (use case‑insensitive comparisons).
- Row numbering: Treat rowNumber as 1‑based for data rows (header at index 0). Map and validate rowNumber → dataRowIdx (dataRowIdx >= 1 && dataRowIdx < tableRows.Count) before indexing tableRows.
File: Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs Lines: 426-506
🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs around lines 426-506, the
PDF table reader currently ignores the password and elementLocateBy parameters
and treats rowNumber as zero-based; open the PDF using
UglyToad.PdfPig.ParsingOptions when a password is provided (pass new
ParsingOptions { Password = password } to PdfPigDocument.Open), use
elementLocateBy to decide matching behavior when locating values (e.g., if
elementLocateBy indicates equals use String.Equals(...,
StringComparison.OrdinalIgnoreCase), if contains use .IndexOf(...,
StringComparison.OrdinalIgnoreCase) >= 0), ensure all header/condition lookups
use case-insensitive comparisons, and treat rowNumber as 1-based relative to the
table where header is row 0 by mapping dataRowIdx = rowNumber (validate
dataRowIdx >= 1 && dataRowIdx < tableRows.Count) before accessing
tableRows[dataRowIdx]; add appropriate checks and log/warn on invalid indices
and return empty string when out of range.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (12)
Ginger/GingerCore/GingerCore.csproj (2)
538-539: Pdfium native packaging: x64-only + very old 2018 build (security/runtime risk).Same concern as previously noted: AnyCPU + x64 native PDFium can crash on 32‑bit; 2018 PDFium binaries are outdated. Either force x64 (preferred) and upgrade to a maintained PDFium native package, or add x86 natives if you must keep AnyCPU.
Also, if you fully migrated to PdfiumViewer, consider removing Freeware.Pdf2Png if unused. Verification script:
#!/bin/bash rg -nP '\bFreeware\.Pdf2Png\b' -g '!**/bin/**' -g '!**/obj/**' || true
534-548: Force x64 target: native OpenCV/PaddleInference are x64-only.With OpenCvSharp.runtime.win and Sdcb.PaddleInference.runtime.win64.mkl, an AnyCPU/x86 process will fail at runtime. Pin the project to x64 (and optionally set RID).
Outside-range patch:
@@ <PropertyGroup> - <PlatformTarget>AnyCPU</PlatformTarget> + <PlatformTarget>x64</PlatformTarget> + <RuntimeIdentifier>win-x64</RuntimeIdentifier> </PropertyGroup>Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (10)
63-69: Don’t serialize all OCR calls with a global lock.This throttles throughput and can deadlock with nested calls. Use a SemaphoreSlim gate or remove synchronization if PaddleOCR is thread‑safe for concurrent Run.
+using System.Threading; @@ - private static PaddleOcrResult SafeRun(Mat mat) - { - lock (lockObject) - { - return Instance.Run(mat); - } - } + private static readonly SemaphoreSlim _ocrGate = new(1, 1); + private static PaddleOcrResult SafeRun(Mat mat) + { + _ocrGate.Wait(); + try { return Instance.Run(mat); } + finally { _ocrGate.Release(); } + }
235-261: Validate and log invalid page tokens/out‑of‑range pages.Current code silently skips bad tokens. Log to aid troubleshooting.
foreach (var p in pageNumbers) { if (int.TryParse(p, out int pageIndex)) { if (pageIndex > 0 && pageIndex <= pdf.PageCount) { using (var img = pdf.Render(pageIndex - 1, dpi, dpi, true)) using (var ms = new MemoryStream()) { img.Save(ms, System.Drawing.Imaging.ImageFormat.Png); list.Add(ms.ToArray()); } } + else + { + Reporter.ToLog(eLogLevel.WARN, $"Requested page {pageIndex} is out of range (1..{pdf.PageCount})."); + } } + else + { + Reporter.ToLog(eLogLevel.WARN, $"Invalid page token '{p}'."); + } }
285-305: Method name misleads: accepts multiple pages.Either rename to ReadTextFromPdfPages/MultiplePages or document that pageNum supports CSV.
307-339: Case‑insensitive label search in PDFs.Match image method; avoids missed hits due to OCR case variance.
- if (region.Text.Contains(label)) + var txt = region.Text ?? string.Empty; + int indexOf = txt.IndexOf(label, StringComparison.OrdinalIgnoreCase); + if (indexOf >= 0) { - int indexOf = region.Text.IndexOf(label) + label.Length; - resultTxt = region.Text.Substring(indexOf).Trim(); + resultTxt = txt.Substring(indexOf + label.Length).Trim(); return resultTxt; }
341-402: Bug: always returns empty string even when partial text was collected; also make searches case‑insensitive.Return the partial result when second label isn’t found.
- return string.Empty; + if (resultTxt.Length > 0) + { + err ??= "Second label not found."; + return resultTxt.ToString(); + } + return string.Empty;Also update IndexOf calls to use StringComparison.OrdinalIgnoreCase as in the image variant.
404-425: Use Dictionary<string, string> (values are strings).Tighten types and simplify consumers.
- public static Dictionary<string, object> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null) + public static Dictionary<string, string> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null) { - var result = new Dictionary<string, object>(); + var result = new Dictionary<string, string>(); @@ - result[$"Page_{i + 1}"] = string.Join(Environment.NewLine, ocrResult.Regions.Select(r => r.Text)); + result[$"Page_{i + 1}"] = string.Join(Environment.NewLine, ocrResult.Regions.Select(r => r.Text));
427-507: PDF table extraction: honor password, use elementLocateBy, and clarify row indexing.
- Open with ParsingOptions when password provided.
- Implement Equals vs Contains based on elementLocateBy.
- Treat rowNumber as 1‑based for data rows (header at 0) or document current behavior.
- using (PdfPigDocument document = PdfPigDocument.Open(pdfFilePath)) + var opts = string.IsNullOrEmpty(password) ? null : new UglyToad.PdfPig.ParsingOptions { Password = password }; + using (PdfPigDocument document = opts == null ? PdfPigDocument.Open(pdfFilePath) : PdfPigDocument.Open(pdfFilePath, opts)) { int pageIndex = string.IsNullOrEmpty(pageNumber) ? 1 : int.Parse(pageNumber); Page page = document.GetPage(pageIndex); @@ - if (useRowNumber) + if (useRowNumber) { - if (rowNumber < tableRows.Count) + int dataRowIdx = Math.Max(1, rowNumber); // header is 0 + if (dataRowIdx < tableRows.Count) { - var targetRow = tableRows[rowNumber]; + var targetRow = tableRows[dataRowIdx]; if (colIndex < targetRow.Count) return targetRow[colIndex].Text; } } else if (!string.IsNullOrEmpty(conditionColumnName) && !string.IsNullOrEmpty(conditionColumnValue)) { // Locate condition column index int condIndex = tableRows.First().FindIndex(w => w.Text.Equals(conditionColumnName, StringComparison.OrdinalIgnoreCase)); if (condIndex != -1) { foreach (var row in tableRows.Skip(1)) { - if (condIndex < row.Count && row[condIndex].Text.Contains(conditionColumnValue)) + if (condIndex < row.Count) { - if (colIndex < row.Count) - return row[colIndex].Text; + bool match = elementLocateBy == ActOcr.eTableElementRunColOperator.Equals + ? row[condIndex].Text.Equals(conditionColumnValue, StringComparison.OrdinalIgnoreCase) + : row[condIndex].Text?.IndexOf(conditionColumnValue, StringComparison.OrdinalIgnoreCase) >= 0; + if (match && colIndex < row.Count) + return row[colIndex].Text; } } } }
147-163: Unify byte[] path: decode directly to Mat (drop MemoryStream + Bitmap).Removes extra conversions and GDI+ dependency here.
- private static PaddleOcrResult ReadOcrResultFromByteArray(byte[] byteArray) + private static PaddleOcrResult ReadOcrResultFromByteArray(byte[] byteArray) { try { - using (var ms = new MemoryStream(byteArray)) - using (var img = (Bitmap)Image.FromStream(ms)) - using (Mat mat = OpenCvSharp.Extensions.BitmapConverter.ToMat(img)) - { - return SafeRun(mat); - } + using (Mat mat = Cv2.ImDecode(byteArray, ImreadModes.Color)) + { + if (mat.Empty()) return null; + return SafeRun(mat); + } } catch (Exception ex) { Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); return null; } }
165-223: Make label matching case‑insensitive and set informative errors.Robust against OCR case variance; report when labels aren’t found.
- if (ocrResult == null || string.IsNullOrWhiteSpace(ocrResult.Text)) + if (ocrResult == null || string.IsNullOrWhiteSpace(ocrResult.Text)) { err = "OCR result is empty."; return string.Empty; } @@ - foreach (var region in ocrResult.Regions) + foreach (var region in ocrResult.Regions) { - string lineTxt = region.Text; + string lineTxt = region.Text ?? string.Empty; @@ - int firstIndexOf = lineTxt.IndexOf(firstLabel); + int firstIndexOf = lineTxt.IndexOf(firstLabel, StringComparison.OrdinalIgnoreCase); if (firstIndexOf != -1) { startedReading = true; - int secondIndexOf = lineTxt.IndexOf(secondLabel, firstIndexOf + firstLabel.Length); + int secondIndexOf = lineTxt.IndexOf(secondLabel, firstIndexOf + firstLabel.Length, StringComparison.OrdinalIgnoreCase); @@ } } else { - int secondIndexOf = lineTxt.IndexOf(secondLabel); + int secondIndexOf = lineTxt.IndexOf(secondLabel, StringComparison.OrdinalIgnoreCase); if (secondIndexOf != -1) { resultTxt.Append(lineTxt.Substring(0, secondIndexOf)); break; } else { resultTxt.Append(lineTxt); } } } + if (resultTxt.Length > 0) + { + err ??= "Second label not found."; + } + else + { + err ??= "First label not found."; + }
134-139: Guard against failed image decode.ImDecode can return an empty Mat; bail early with a clear log.
- using (Mat src = Cv2.ImDecode(byteArray, ImreadModes.Color)) + using (Mat src = Cv2.ImDecode(byteArray, ImreadModes.Color)) { + if (src.Empty()) + { + Reporter.ToLog(eLogLevel.ERROR, "Failed to decode image from byte array."); + return string.Empty; + } var result = SafeRun(src); txtOutput = result.Text; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Ginger/GingerCore/tessdata/pdf.ttfis excluded by!**/*.ttf,!**/*.ttf
📒 Files selected for processing (2)
Ginger/GingerCore/GingerCore.csproj(2 hunks)Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-25T08:48:11.915Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCore/GingerCore.csproj:538-538
Timestamp: 2025-08-25T08:48:11.915Z
Learning: In the Ginger codebase, the OWASPZAPDotNetAPI package reference in GingerCore.csproj is required for architectural reasons, even when direct usage is not immediately visible in the codebase. This was confirmed by AmanPrasad43 during the security testing feature implementation.
Applied to files:
Ginger/GingerCore/GingerCore.csproj
📚 Learning: 2025-09-05T09:30:10.074Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:5942-5960
Timestamp: 2025-09-05T09:30:10.074Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (around SVG helpers, approx. lines 5942–5960 and 5983–5984), maintainer prefers keeping ToLower() for tag/name normalization; do not suggest replacing with ToLowerInvariant() in future reviews for this area.
Applied to files:
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs
📚 Learning: 2025-08-29T09:35:46.020Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs:287-309
Timestamp: 2025-08-29T09:35:46.020Z
Learning: In Ginger ZAP security testing, alert names should preserve their original formatting with spaces and hyphens. When comparing alert names in EvaluateScanResultAPI and similar methods, use StringComparison.OrdinalIgnoreCase for case-insensitive matching but do not normalize by removing spaces or hyphens, as alert names like "False Positive" need to remain as separate words. Confirmed by AmanPrasad43 in PR #4286.
Applied to files:
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs
🧬 Code graph analysis (1)
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (1)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
🔇 Additional comments (2)
Ginger/GingerCore/GingerCore.csproj (2)
544-548: Trim Sdcb.PaddleOCR.Models.Shared if unusedGinger/GingerCore/GingerCore.csproj (lines 544–548) references Sdcb.PaddleOCR.Models.Local and Sdcb.PaddleOCR.Models.Shared (v2.7.0.1). If you only use Local models (e.g., LocalFullModels.EnglishV4) remove the Sdcb.PaddleOCR.Models.Shared PackageReference to reduce footprint. Verify with a repo search (example): git grep -n -e 'PaddleOCR.Models.Shared' -e 'LocalFullModels' -e 'EnglishV4' || rg -n --hidden -uu -S 'PaddleOCR.Models.Shared|LocalFullModels|EnglishV4' --glob '!/bin/' --glob '!/obj/' || true
48-53: Remove Tesseract PackageReference from Ginger/GingerCore/GingerCore.csprojtessdata was removed and repository search found no usages of the Tesseract API outside this PackageReference — remove it to eliminate a dead dependency.
Proposed removal (outside the selected range):
- <PackageReference Include="Tesseract"> - <Version>5.2.0</Version> - </PackageReference>
| private static List<string> GetListOfPageNos(string pageNumber) | ||
| { | ||
| try | ||
| { | ||
| using (Pix img = Pix.LoadFromMemory(byteArray)) | ||
| { | ||
| Page page = Instance.Process(img); | ||
| return page; | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); | ||
| } | ||
| return null; | ||
| return pageNumber.Split(',') | ||
| .Select(p => p.Trim()) | ||
| .Where(p => !string.IsNullOrEmpty(p)) | ||
| .ToList(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Handle null/empty page lists defensively.
Minor: avoid NRE when pageNumber is null/whitespace.
- return pageNumber.Split(',')
+ if (string.IsNullOrWhiteSpace(pageNumber)) return new List<string>();
+ return pageNumber.Split(',')
.Select(p => p.Trim())
.Where(p => !string.IsNullOrEmpty(p))
.ToList();📝 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.
| private static List<string> GetListOfPageNos(string pageNumber) | |
| { | |
| try | |
| { | |
| using (Pix img = Pix.LoadFromMemory(byteArray)) | |
| { | |
| Page page = Instance.Process(img); | |
| return page; | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); | |
| } | |
| return null; | |
| return pageNumber.Split(',') | |
| .Select(p => p.Trim()) | |
| .Where(p => !string.IsNullOrEmpty(p)) | |
| .ToList(); | |
| } | |
| private static List<string> GetListOfPageNos(string pageNumber) | |
| { | |
| if (string.IsNullOrWhiteSpace(pageNumber)) return new List<string>(); | |
| return pageNumber.Split(',') | |
| .Select(p => p.Trim()) | |
| .Where(p => !string.IsNullOrEmpty(p)) | |
| .ToList(); | |
| } |
🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs around lines 227 to 233,
the GetListOfPageNos method can throw a null reference when pageNumber is null
or only whitespace; update the method to defensively handle
null/empty/whitespace input by returning an empty List<string> when
string.IsNullOrWhiteSpace(pageNumber) is true, otherwise perform the existing
Split/Trim/Where/ToList processing.
| private static void GetTableDataFromPageArea(string columnName, bool useRowNumber, int rowNumber, string conditionColumnName, string conditionColumnValue, ref string txtOutput, Table table, ActOcr.eTableElementRunColOperator elementLocateBy) | ||
| { | ||
| List<string> lstPageNos = []; | ||
| try | ||
| var rows = table.Rows; | ||
| if (rows.Count == 0) return; | ||
|
|
||
| var header = rows.First().Select(cell => cell.GetText()).ToList(); | ||
| if (!header.Contains(columnName)) return; | ||
|
|
||
| int colIndex = header.IndexOf(columnName); | ||
|
|
||
| for (int i = 1; i < rows.Count; i++) | ||
| { | ||
| bool isParse = true; | ||
| var row = rows[i]; | ||
|
|
||
| if (pageNumber.Contains("-")) | ||
| if (useRowNumber && i == rowNumber) | ||
| { | ||
| string[] pageArray = pageNumber.Split('-'); | ||
| int i, j = 0; | ||
| isParse = int.TryParse(pageArray[0], out i); | ||
| if (isParse) | ||
| { | ||
| isParse = int.TryParse(pageArray[1], out j); | ||
| } | ||
| if (isParse) | ||
| { | ||
| for (; i <= j; i++) | ||
| { | ||
| lstPageNos.Add(i.ToString()); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticErrorMessage, "Invalid Page Number"); | ||
| return null; | ||
| } | ||
| txtOutput = row[colIndex].GetText(); | ||
| break; | ||
| } | ||
| else if (pageNumber.Contains(",")) | ||
| { | ||
| string[] pageArray = pageNumber.Split(','); | ||
| for (int i = 0; i < pageArray.Length; i++) | ||
| { | ||
| int j = 0; | ||
| isParse = int.TryParse(pageArray[i], out j); | ||
| if (!isParse) | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticErrorMessage, "Invalid Page Number"); | ||
| return null; | ||
| } | ||
| lstPageNos.Add(pageArray[i]); | ||
| } | ||
| } | ||
| else | ||
|
|
||
| if (!string.IsNullOrEmpty(conditionColumnName) && header.Contains(conditionColumnName)) | ||
| { | ||
| int j = 0; | ||
| isParse = int.TryParse(pageNumber, out j); | ||
| if (!isParse) | ||
| int condIndex = header.IndexOf(conditionColumnName); | ||
| if (row[condIndex].GetText().Equals(conditionColumnValue, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticErrorMessage, "Invalid Page Number"); | ||
| return null; | ||
| txtOutput = row[colIndex].GetText(); | ||
| break; | ||
| } | ||
| lstPageNos.Add(pageNumber); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); | ||
| return null; | ||
| } | ||
| return lstPageNos; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Dead code: remove Tabula helper and the Tabula dependency.
GetTableDataFromPageArea is unused after the PdfPig migration. Delete the method and drop the Tabula package from the project to reduce surface area.
In this file, remove the method entirely. In the csproj (outside-range change):
- <PackageReference Include="Tabula">
- <Version>0.1.2</Version>
- </PackageReference>Verification:
#!/bin/bash
rg -nP '\bTabula\b|GetTableDataFromPageArea' -g '!**/bin/**' -g '!**/obj/**' || true🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs around lines 509-539: the
GetTableDataFromPageArea method (Tabula helper) is dead code after the PdfPig
migration; delete this entire private method from the file, remove any
using/imports that become unused as a result, and remove the Tabula package
reference from the project file (.csproj). After removal, run the provided
ripgrep verification command to ensure no remaining references to "Tabula" or
"GetTableDataFromPageArea" (exclude bin/ and obj/ directories).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (9)
145-161: Inconsistent byte[] decoding approach.This method still uses the inefficient
MemoryStream→Bitmap→Matconversion, whileReadTextFromByteArray(lines 132-136) correctly usesCv2.ImDecode.Apply the same optimization used in
ReadTextFromByteArray:- try - { - using (var ms = new MemoryStream(byteArray)) - using (var img = (Bitmap)Image.FromStream(ms)) - using (Mat mat = OpenCvSharp.Extensions.BitmapConverter.ToMat(img)) - { - return SafeRun(mat); - } - } + try + { + using (Mat mat = Cv2.ImDecode(byteArray, ImreadModes.Color)) + { + return SafeRun(mat); + } + }
182-210: Case-insensitive for first label but not for second label.The first label search uses
OrdinalIgnoreCase(line 182), but the second label search (line 201) uses the default case-sensitive comparison.Fix the inconsistency by making both comparisons case-insensitive:
- int secondIndexOf = lineTxt.IndexOf(secondLabel); + int secondIndexOf = lineTxt.IndexOf(secondLabel, StringComparison.OrdinalIgnoreCase);
225-231: Handle null/empty pageNumber defensively.The method doesn't check for null pageNumber before calling Split, which could cause a NullReferenceException.
private static List<string> GetListOfPageNos(string pageNumber) { + if (string.IsNullOrWhiteSpace(pageNumber)) + return new List<string>(); return pageNumber.Split(',') .Select(p => p.Trim()) .Where(p => !string.IsNullOrEmpty(p)) .ToList(); }
322-327: Inconsistent case sensitivity in PDF label search.The PDF label search uses case-sensitive
ContainsandIndexOf, while the image version (lines 101-107) correctly uses case-insensitive matching.- if (region.Text.Contains(label)) + var txt = region.Text ?? string.Empty; + int indexOf = txt.IndexOf(label, StringComparison.OrdinalIgnoreCase); + if (indexOf >= 0) { - int indexOf = region.Text.IndexOf(label) + label.Length; - resultTxt = region.Text.Substring(indexOf).Trim(); + resultTxt = txt.Substring(indexOf + label.Length).Trim(); return resultTxt; }
398-400: Always returns empty string instead of accumulated text.When text is collected but the second label isn't found, the method returns empty string instead of the partial result. This differs from the image version behavior.
- return string.Empty; + if (resultTxt.Length > 0) + { + err ??= "Second label not found."; + return resultTxt.ToString(); + } + return string.Empty;
402-423: Use strongly-typed Dictionary<string, string> instead of object.All values stored are strings, so using
objectas the value type is unnecessary and loses type safety.-public static Dictionary<string, object> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null) +public static Dictionary<string, string> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null) { - var result = new Dictionary<string, object>(); + var result = new Dictionary<string, string>();
439-502: PDF table extraction ignores password and elementLocateBy parameters.
- The password parameter is never used when opening the PDF
- The elementLocateBy parameter (equals vs contains) is ignored - only Contains is used (line 490)
- Row numbering appears to be 0-based internally but might be expected as 1-based from UI
Apply these fixes:
- using (PdfPigDocument document = PdfPigDocument.Open(pdfFilePath)) + var opts = string.IsNullOrEmpty(password) ? null : new UglyToad.PdfPig.ParsingOptions() { Password = password }; + using (PdfPigDocument document = opts == null ? PdfPigDocument.Open(pdfFilePath) : PdfPigDocument.Open(pdfFilePath, opts)) ... if (useRowNumber) { - if (rowNumber < tableRows.Count) + // Treat rowNumber as 1-based (row 0 is header) + if (rowNumber > 0 && rowNumber < tableRows.Count) { var targetRow = tableRows[rowNumber]; ... - if (condIndex < row.Count && row[condIndex].Text.Contains(conditionColumnValue)) + if (condIndex < row.Count) + { + bool match = elementLocateBy == ActOcr.eTableElementRunColOperator.Equals + ? row[condIndex].Text.Equals(conditionColumnValue, StringComparison.OrdinalIgnoreCase) + : row[condIndex].Text.IndexOf(conditionColumnValue, StringComparison.OrdinalIgnoreCase) >= 0; + if (match && colIndex < row.Count) + return row[colIndex].Text; + }Let me verify that PdfPig supports password-protected PDFs with the correct API:
UglyToad.PdfPig ParsingOptions Password API latest version
511-544: Dead code: Remove unused Tabula helper method.The
GetTableDataFromPageAreamethod is dead code after migrating to PdfPig. It should be removed along with the Tabula dependency.Delete this entire method and remove the Tabula import (line 30). Also remove the Tabula NuGet package from the project file.
#!/bin/bash # Verify no other references to Tabula or this method exist rg -n 'GetTableDataFromPageArea|Tabula' --type cs --type xml -g '!**/bin/**' -g '!**/obj/**'
361-389: Missing case-insensitive comparisons in PDF between-labels extraction.Both label searches use case-sensitive IndexOf, inconsistent with the image version.
Apply case-insensitive matching:
- int idx1 = lineTxt.IndexOf(firstLabel); + int idx1 = lineTxt.IndexOf(firstLabel, StringComparison.OrdinalIgnoreCase); ... - int idx2 = lineTxt.IndexOf(secondLabel, idx1 + firstLabel.Length); + int idx2 = lineTxt.IndexOf(secondLabel, idx1 + firstLabel.Length, StringComparison.OrdinalIgnoreCase); ... - int idx2 = lineTxt.IndexOf(secondLabel); + int idx2 = lineTxt.IndexOf(secondLabel, StringComparison.OrdinalIgnoreCase);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T09:30:10.074Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:5942-5960
Timestamp: 2025-09-05T09:30:10.074Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (around SVG helpers, approx. lines 5942–5960 and 5983–5984), maintainer prefers keeping ToLower() for tag/name normalization; do not suggest replacing with ToLowerInvariant() in future reviews for this area.
Applied to files:
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs
📚 Learning: 2025-08-29T09:35:46.020Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs:287-309
Timestamp: 2025-08-29T09:35:46.020Z
Learning: In Ginger ZAP security testing, alert names should preserve their original formatting with spaces and hyphens. When comparing alert names in EvaluateScanResultAPI and similar methods, use StringComparison.OrdinalIgnoreCase for case-insensitive matching but do not normalize by removing spaces or hyphens, as alert names like "False Positive" need to remain as separate words. Confirmed by AmanPrasad43 in PR #4286.
Applied to files:
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs
🧬 Code graph analysis (1)
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (1)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
🔇 Additional comments (4)
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (4)
63-69: Excellent refactor: SemaphoreSlim is a much better choice than lock.Using
SemaphoreSlim(1, 1)for thread synchronization is indeed better than the coarse lock (mentioned in past reviews). This allows for potential async operations in the future and is more efficient for I/O-bound work.
75-81: LGTM! Direct Mat loading is more efficient.The refactor from
Bitmap→Matto directly loading viaCv2.ImReadeliminates unnecessary GDI+ allocations and file handle pinning. This is exactly the optimization mentioned in previous reviews.
95-108: Great improvement on case-insensitive label matching!The implementation now correctly uses
StringComparison.OrdinalIgnoreCasefor case-insensitive matching and handles potential null Text gracefully.
132-136: Perfect! Direct byte[] to Mat decoding.Using
Cv2.ImDecodedirectly instead of theMemoryStream→Bitmap→Matconversion eliminates unnecessary intermediate allocations.
| public static PaddleOcrAll Instance | ||
| { | ||
| get | ||
| { | ||
| lock (lockObject) | ||
| { | ||
| if (instance == null) | ||
| { | ||
| string exeFilePath = Path.GetDirectoryName(typeof(GingerOcrOperations).Assembly.Location); | ||
| string tessDataFilePath = Path.Combine(exeFilePath, "tessdata"); | ||
| instance = new TesseractEngine(tessDataFilePath, "eng", EngineMode.Default); | ||
| FullOcrModel model = LocalFullModels.EnglishV4; | ||
| instance = new PaddleOcrAll(model) | ||
| { | ||
| AllowRotateDetection = true, | ||
| Enable180Classification = false, | ||
|
|
||
| }; | ||
| } | ||
| return instance; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Good singleton initialization, but fix the dangling blank line.
The thread-safe singleton implementation using double-checked locking is solid. The PaddleOCR settings (rotation detection enabled, 180-degree classification disabled) are appropriate for general OCR use cases.
Remove the unnecessary blank line:
AllowRotateDetection = true,
Enable180Classification = false,
-
};📝 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.
| public static PaddleOcrAll Instance | |
| { | |
| get | |
| { | |
| lock (lockObject) | |
| { | |
| if (instance == null) | |
| { | |
| string exeFilePath = Path.GetDirectoryName(typeof(GingerOcrOperations).Assembly.Location); | |
| string tessDataFilePath = Path.Combine(exeFilePath, "tessdata"); | |
| instance = new TesseractEngine(tessDataFilePath, "eng", EngineMode.Default); | |
| FullOcrModel model = LocalFullModels.EnglishV4; | |
| instance = new PaddleOcrAll(model) | |
| { | |
| AllowRotateDetection = true, | |
| Enable180Classification = false, | |
| }; | |
| } | |
| return instance; | |
| } | |
| } | |
| } | |
| public static PaddleOcrAll Instance | |
| { | |
| get | |
| { | |
| lock (lockObject) | |
| { | |
| if (instance == null) | |
| { | |
| FullOcrModel model = LocalFullModels.EnglishV4; | |
| instance = new PaddleOcrAll(model) | |
| { | |
| AllowRotateDetection = true, | |
| Enable180Classification = false, | |
| }; | |
| } | |
| return instance; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs around lines 42 to 61,
there's an unnecessary blank line inside the object initializer for the
PaddleOcrAll instance; remove that dangling empty line so the initializer ends
immediately after Enable180Classification = false, keeping formatting compact
and consistent.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
Ginger/GingerCore/GingerCore.csproj (1)
538-539: Blocking: x64-only PDFium native on AnyCPU + very outdated/vulnerable build.
- The project targets AnyCPU (Line 41) but references PdfiumViewer.Native.x86_64.v8-xfa (x64-only). A 32‑bit process will fail to load native PDFium.
- Version 2018.4.8.256 is >7 years old and associated with multiple PDFium CVEs from 2016–2018. Move to maintained packaging with current binaries and align platform. Options: force x64 for this project, or add x86 native package and keep AnyCPU. Prefer updating to a newer PdfiumViewer fork/variant that ships recent PDFium. (nuget.org)
Proposed change (if x64-only is acceptable):
- <PlatformTarget>AnyCPU</PlatformTarget> + <PlatformTarget>x64</PlatformTarget>And replace the 2018 native package with a modern equivalent.
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (8)
146-152: Avoid GDI+ hop for byte[] → Mat.Decode bytes directly with Cv2.ImDecode to drop Bitmap/BitmapConverter overhead.
- try - { - using (var ms = new MemoryStream(byteArray)) - using (var img = (Bitmap)Image.FromStream(ms)) - using (Mat mat = OpenCvSharp.Extensions.BitmapConverter.ToMat(img)) - { - return Instance.Run(mat); - } - } + try + { + using (Mat mat = Cv2.ImDecode(byteArray, ImreadModes.Color)) + { + return Instance.Run(mat); + } + }
280-300: Method name vs. behavior: accepts multiple pages.ReadTextFromPdfSinglePage accepts comma‑separated pages. Rename to ReadTextFromPdfPages or document that it supports multiple pages to avoid confusion.
319-324: Case‑insensitive label search in PDFs.Use IndexOf with OrdinalIgnoreCase and reuse the index for Substring.
- if (region.Text.Contains(label)) + var txt = region.Text ?? string.Empty; + int indexOf = txt.IndexOf(label, StringComparison.OrdinalIgnoreCase); + if (indexOf >= 0) { - int indexOf = region.Text.IndexOf(label) + label.Length; - resultTxt = region.Text.Substring(indexOf).Trim(); + resultTxt = txt.Substring(indexOf + label.Length).Trim(); return resultTxt; }
222-228: Guard against null/empty page lists.Prevent NRE when pageNumber is null/whitespace.
- return pageNumber.Split(',') + if (string.IsNullOrWhiteSpace(pageNumber)) return new List<string>(); + return pageNumber.Split(',')
198-205: Make second-label search case‑insensitive.Else-branch still uses case‑sensitive IndexOf; align with OrdinalIgnoreCase.
- int secondIndexOf = lineTxt.IndexOf(secondLabel); + int secondIndexOf = lineTxt.IndexOf(secondLabel, StringComparison.OrdinalIgnoreCase);
235-255: Improve diagnostics for invalid page tokens/out-of-range pages.Log warnings on non‑numeric tokens and out‑of‑range indices to aid users.
- foreach (var p in pageNumbers) + foreach (var p in pageNumbers) { - if (int.TryParse(p, out int pageIndex)) + if (int.TryParse(p, out int pageIndex)) { if (pageIndex > 0 && pageIndex <= pdf.PageCount) { using (var img = pdf.Render(pageIndex - 1, dpi, dpi, true)) using (var ms = new MemoryStream()) { img.Save(ms, System.Drawing.Imaging.ImageFormat.Png); list.Add(ms.ToArray()); } } + else + { + Reporter.ToLog(eLogLevel.WARN, $"Requested page {pageIndex} is out of range (1..{pdf.PageCount})."); + } } + else + { + Reporter.ToLog(eLogLevel.WARN, $"Invalid page token '{p}'."); + } }
352-397: Bug: always returns empty even when text was collected; also case‑sensitive searches.
- Case‑sensitive IndexOf calls can miss labels.
- Final return unconditionally returns string.Empty, discarding partial result. Fix both.
- foreach (var region in ocrResult.Regions) + foreach (var region in ocrResult.Regions) { - string lineTxt = region.Text; + string lineTxt = region.Text ?? string.Empty; if (!started) { - int idx1 = lineTxt.IndexOf(firstLabel); + int idx1 = lineTxt.IndexOf(firstLabel, StringComparison.OrdinalIgnoreCase); if (idx1 != -1) { started = true; - int idx2 = lineTxt.IndexOf(secondLabel, idx1 + firstLabel.Length); + int idx2 = lineTxt.IndexOf(secondLabel, idx1 + firstLabel.Length, StringComparison.OrdinalIgnoreCase); if (idx2 != -1) { resultTxt.Append(lineTxt.Substring(idx1 + firstLabel.Length, idx2 - (idx1 + firstLabel.Length))); - return resultTxt.ToString(); + return resultTxt.ToString(); } else { resultTxt.Append(lineTxt.Substring(idx1 + firstLabel.Length)); } } } else { - int idx2 = lineTxt.IndexOf(secondLabel); + int idx2 = lineTxt.IndexOf(secondLabel, StringComparison.OrdinalIgnoreCase); if (idx2 != -1) { resultTxt.Append(lineTxt.Substring(0, idx2)); - return resultTxt.ToString(); + return resultTxt.ToString(); } else { resultTxt.Append(lineTxt); } } } @@ - return string.Empty; + if (resultTxt.Length > 0) + { + err ??= "Second label not found."; + return resultTxt.ToString(); + } + return string.Empty;
436-499: PDF table extraction: support passwords, use elementLocateBy, and fix row indexing.
- Open encrypted PDFs via ParsingOptions.Password/Passwords.
- Respect elementLocateBy (Equals vs Contains, case‑insensitive).
- Treat rowNumber as 1‑based for data rows (row 0 as header). (github.com)
- using (PdfPigDocument document = PdfPigDocument.Open(pdfFilePath)) + var parsing = string.IsNullOrEmpty(password) ? null : new UglyToad.PdfPig.ParsingOptions { Password = password }; + using (PdfPigDocument document = parsing == null ? PdfPigDocument.Open(pdfFilePath) : PdfPigDocument.Open(pdfFilePath, parsing)) { - int pageIndex = string.IsNullOrEmpty(pageNumber) ? 1 : int.Parse(pageNumber); + int pageIndex = string.IsNullOrEmpty(pageNumber) ? 1 : int.Parse(pageNumber); Page page = document.GetPage(pageIndex); @@ - if (useRowNumber) + if (useRowNumber) { - if (rowNumber < tableRows.Count) + // data rows start at index 1 (index 0 is header) + int dataRowIdx = Math.Max(1, rowNumber); + if (dataRowIdx < tableRows.Count) { - var targetRow = tableRows[rowNumber]; + var targetRow = tableRows[dataRowIdx]; if (colIndex < targetRow.Count) { return targetRow[colIndex].Text; } } } else if (!string.IsNullOrEmpty(conditionColumnName) && !string.IsNullOrEmpty(conditionColumnValue)) { // Locate condition column index - int condIndex = tableRows.First().FindIndex(w => w.Text.Equals(conditionColumnName, StringComparison.OrdinalIgnoreCase)); + int condIndex = tableRows.First().FindIndex(w => w.Text.Equals(conditionColumnName, StringComparison.OrdinalIgnoreCase)); if (condIndex != -1) { foreach (var row in tableRows.Skip(1)) { - if (condIndex < row.Count && row[condIndex].Text.Contains(conditionColumnValue)) + if (condIndex < row.Count) { - if (colIndex < row.Count) - { - return row[colIndex].Text; - } + bool match = elementLocateBy == ActOcr.eTableElementRunColOperator.Equals + ? row[condIndex].Text.Equals(conditionColumnValue, StringComparison.OrdinalIgnoreCase) + : row[condIndex].Text.IndexOf(conditionColumnValue, StringComparison.OrdinalIgnoreCase) >= 0; + if (match && colIndex < row.Count) return row[colIndex].Text; } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCore/GingerCore.csproj(2 hunks)Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:30:10.074Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:5942-5960
Timestamp: 2025-09-05T09:30:10.074Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (around SVG helpers, approx. lines 5942–5960 and 5983–5984), maintainer prefers keeping ToLower() for tag/name normalization; do not suggest replacing with ToLowerInvariant() in future reviews for this area.
Applied to files:
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs
📚 Learning: 2025-08-29T09:35:46.020Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs:287-309
Timestamp: 2025-08-29T09:35:46.020Z
Learning: In Ginger ZAP security testing, alert names should preserve their original formatting with spaces and hyphens. When comparing alert names in EvaluateScanResultAPI and similar methods, use StringComparison.OrdinalIgnoreCase for case-insensitive matching but do not normalize by removing spaces or hyphens, as alert names like "False Positive" need to remain as separate words. Confirmed by AmanPrasad43 in PR #4286.
Applied to files:
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs
📚 Learning: 2025-08-25T08:48:11.915Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCore/GingerCore.csproj:538-538
Timestamp: 2025-08-25T08:48:11.915Z
Learning: In the Ginger codebase, the OWASPZAPDotNetAPI package reference in GingerCore.csproj is required for architectural reasons, even when direct usage is not immediately visible in the codebase. This was confirmed by AmanPrasad43 during the security testing feature implementation.
Applied to files:
Ginger/GingerCore/GingerCore.csproj
🧬 Code graph analysis (1)
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (1)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
🔇 Additional comments (2)
Ginger/GingerCore/GingerCore.csproj (1)
534-536: OpenCV packaging looks correct for Windows.Using OpenCvSharp4 + runtime.win is the right choice for native binding resolution on Windows.
Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (1)
399-419: Change return type to Dictionary<string,string>; verify external callersAll returned values are strings — change the method signature and local variable to Dictionary<string, string>. Repo search returned only the declaration (Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs:399–401); no internal callers found. If this is a public API, add a strongly‑typed overload and deprecate the object-returning method instead of breaking callers. Apply the diff below if safe.
- public static Dictionary<string, object> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null) + public static Dictionary<string, string> ReadTextFromPdfAllPages(string pdfFilePath, int dpi, string password = null) { - var result = new Dictionary<string, object>(); + var result = new Dictionary<string, string>(); @@ - result[$"Page_{i + 1}"] = string.Join(Environment.NewLine, ocrResult.Regions.Select(r => r.Text)); + result[$"Page_{i + 1}"] = string.Join(Environment.NewLine, ocrResult.Regions.Select(r => r.Text)); } - return result; + return result;
| <EmbeddedResource Remove="tessdata\**" /> | ||
| <None Remove="Actions\REST\**" /> | ||
| <None Remove="Actions\WebServices\**" /> | ||
| <None Remove="Environments\**" /> | ||
| <None Remove="Plugins\**" /> | ||
| <None Remove="tessdata\**" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
🧹 Nitpick
Good cleanup: remove tessdata assets.
This aligns with the PaddleOCR migration. As a follow-up, consider removing the Tesseract NuGet reference if no longer used to reduce footprint (lines 596–598).
🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerCore.csproj around lines 48-54 you removed tessdata
assets; as a follow-up remove the Tesseract NuGet reference mentioned on lines
596–598 to avoid shipping unused dependency: delete the PackageReference (and
any associated Tesseract-related targets/props entries) for Tesseract from the
csproj, run a solution-wide search to ensure no remaining references/usages
remain (or update code to use PaddleOCR equivalents), and then build the project
to verify no missing references.
| <PackageReference Include="Sdcb.PaddleInference" Version="3.0.1" /> | ||
| <PackageReference Include="Sdcb.PaddleInference.runtime.win64.mkl" Version="3.0.0.36" /> | ||
| <PackageReference Include="Sdcb.PaddleOCR" Version="3.0.0" /> | ||
| <PackageReference Include="Sdcb.PaddleOCR.Models.Local" Version="3.0.1" /> | ||
| <PackageReference Include="Sdcb.PaddleOCR.Models.Shared" Version="2.7.0.1" /> |
There was a problem hiding this comment.
🧹 Nitpick
Version alignment: bump Sdcb.PaddleOCR to 3.0.1 for consistency.
You’re on Sdcb.PaddleInference 3.0.1 and Models.Local 3.0.1 but Sdcb.PaddleOCR is 3.0.0. Recommend upgrading Sdcb.PaddleOCR to 3.0.1 to avoid API skew. (nuget.org)
- <PackageReference Include="Sdcb.PaddleOCR" Version="3.0.0" />
+ <PackageReference Include="Sdcb.PaddleOCR" Version="3.0.1" />🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerCore.csproj around lines 544 to 548, the
Sdcb.PaddleOCR package is at version 3.0.0 while Sdcb.PaddleInference and
Sdcb.PaddleOCR.Models.Local are at 3.0.1 causing a version skew; update the
Sdcb.PaddleOCR PackageReference to Version="3.0.1" to align versions, then run
dotnet restore and a build to verify no API or binding issues and adjust any
code if the minor-version change surfaces breaking API differences.
| public static PaddleOcrAll Instance | ||
| { | ||
| get | ||
| { | ||
| lock (lockObject) | ||
| { | ||
| if (instance == null) | ||
| { | ||
| string exeFilePath = Path.GetDirectoryName(typeof(GingerOcrOperations).Assembly.Location); | ||
| string tessDataFilePath = Path.Combine(exeFilePath, "tessdata"); | ||
| instance = new TesseractEngine(tessDataFilePath, "eng", EngineMode.Default); | ||
| FullOcrModel model = LocalFullModels.EnglishV4; | ||
| instance = new PaddleOcrAll(model) | ||
| { | ||
| AllowRotateDetection = true, | ||
| Enable180Classification = false, | ||
|
|
||
|
|
||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify model identifier and address potential concurrency.
- Confirm that LocalFullModels.EnglishV4 exists in Sdcb.PaddleOCR.Models.Local 3.0.1. If not, switch to an available model (e.g., EnglishV3 or a LocalV4 symbol). (nuget.org)
- If PaddleOcrAll isn’t thread‑safe, concurrent calls to Instance.Run can race. Consider gating with a SemaphoreSlim or creating one instance per thread.
🌐 Web query:
Does Sdcb.PaddleOCR.Models.Local 3.0.1 expose LocalFullModels.EnglishV4, and is PaddleOcrAll.Run thread-safe when used from multiple threads with a single instance?
💡 Result:
Short answers:
- Yes — the Local models package exposes LocalFullModels including EnglishV4 (used in examples/issues). [1][2]
- No — PaddleOcrAll.Run is not documented as safe for concurrent calls on a single instance; the project provides QueuedPaddleOcrAll (or creating separate PaddleOcrAll instances per thread) for multi-threaded use, and there are reported crashes when calling PaddleOcrAll.Run concurrently (use the queue wrapper for thread-safe/queued inference, especially on GPU). [1][3][4]
Sources:
- PaddleSharp OCR docs (LocalFullModels usage / package info). [turn2search0]
- NuGet: Sdcb.PaddleOCR.Models.Local 3.0.1 (package metadata). [turn1search1]
- GitHub issue showing use of LocalFullModels.EnglishV4 and concurrent-run crashes / ExecutionEngineException. [turn6search3]
- GitHub issues and release notes recommending QueuedPaddleOcrAll for multi-threaded use. [turn6search0][turn5search5]
LocalFullModels.EnglishV4 is valid — make PaddleOcrAll usage thread‑safe
- Model: LocalFullModels.EnglishV4 exists in Sdcb.PaddleOCR.Models.Local 3.0.1 — no change needed.
- Concurrency (critical): PaddleOcrAll.Run is not safe for concurrent calls and there are reported crashes. In Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (lines 42–57) replace the singleton PaddleOcrAll usage with a thread‑safe approach — recommended: use QueuedPaddleOcrAll; alternatives: create one PaddleOcrAll per thread or serialize Run calls with a SemaphoreSlim/lock.
🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs around lines 42 to 57, the
current singleton exposes a shared PaddleOcrAll instance which is not safe for
concurrent Run calls and causes crashes; replace the singleton usage with a
thread‑safe approach: instantiate and assign a QueuedPaddleOcrAll configured
with the same model and options (AllowRotateDetection, Enable180Classification,
etc.) instead of PaddleOcrAll, or alternatively create one PaddleOcrAll per
thread (e.g., ThreadLocal) or serialize Run calls by guarding Run with a
dedicated SemaphoreSlim/lock; ensure the replacement preserves the existing
configuration values and that all callers use the new thread‑safe instance or
the serialized Run wrapper.
| private static void GetTableDataFromPageArea(string columnName, bool useRowNumber, int rowNumber, string conditionColumnName, string conditionColumnValue, ref string txtOutput, Table table, ActOcr.eTableElementRunColOperator elementLocateBy) | ||
| { | ||
| List<string> lstPageNos = []; | ||
| try | ||
| var rows = table.Rows; | ||
| if (rows.Count == 0) | ||
| { | ||
| bool isParse = true; | ||
| return; | ||
| } | ||
|
|
||
| if (pageNumber.Contains("-")) | ||
| { | ||
| string[] pageArray = pageNumber.Split('-'); | ||
| int i, j = 0; | ||
| isParse = int.TryParse(pageArray[0], out i); | ||
| if (isParse) | ||
| { | ||
| isParse = int.TryParse(pageArray[1], out j); | ||
| } | ||
| if (isParse) | ||
| { | ||
| for (; i <= j; i++) | ||
| { | ||
| lstPageNos.Add(i.ToString()); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticErrorMessage, "Invalid Page Number"); | ||
| return null; | ||
| } | ||
| } | ||
| else if (pageNumber.Contains(",")) | ||
| var header = rows.First().Select(cell => cell.GetText()).ToList(); | ||
| if (!header.Contains(columnName)) return; | ||
|
|
||
| int colIndex = header.IndexOf(columnName); | ||
|
|
||
| for (int i = 1; i < rows.Count; i++) | ||
| { | ||
| var row = rows[i]; | ||
|
|
||
| if (useRowNumber && i == rowNumber) | ||
| { | ||
| string[] pageArray = pageNumber.Split(','); | ||
| for (int i = 0; i < pageArray.Length; i++) | ||
| { | ||
| int j = 0; | ||
| isParse = int.TryParse(pageArray[i], out j); | ||
| if (!isParse) | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticErrorMessage, "Invalid Page Number"); | ||
| return null; | ||
| } | ||
| lstPageNos.Add(pageArray[i]); | ||
| } | ||
| txtOutput = row[colIndex].GetText(); | ||
| break; | ||
| } | ||
| else | ||
|
|
||
| if (!string.IsNullOrEmpty(conditionColumnName) && header.Contains(conditionColumnName)) | ||
| { | ||
| int j = 0; | ||
| isParse = int.TryParse(pageNumber, out j); | ||
| if (!isParse) | ||
| int condIndex = header.IndexOf(conditionColumnName); | ||
| if (row[condIndex].GetText().Equals(conditionColumnValue, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticErrorMessage, "Invalid Page Number"); | ||
| return null; | ||
| txtOutput = row[colIndex].GetText(); | ||
| break; | ||
| } | ||
| lstPageNos.Add(pageNumber); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, ex.Message, ex); | ||
| return null; | ||
| } | ||
| return lstPageNos; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Dead code + dependency cleanup: remove Tabula helper and package.
GetTableDataFromPageArea is unused after migrating to PdfPig; delete it and drop the Tabula package to reduce surface area.
🏁 Script executed:
#!/bin/bash
# Verify no references remain
rg -nP '\bTabula\b|GetTableDataFromPageArea' -g '!**/bin/**' -g '!**/obj/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
# Recursively search for references to GetTableDataFromPageArea or Tabula in C# source files
rg -nP '\b(GetTableDataFromPageArea|Tabula)\b' -g '*.cs'Length of output: 503
Remove Tabula dead code and package
GetTableDataFromPageArea and the Tabula using remain but are not referenced elsewhere; delete the method and the using Tabula;, and remove the Tabula NuGet/package reference from project files (.csproj / packages.config / Directory.Packages.props).
Locations: Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs — using Tabula; (line 30), table-extraction comment (line 422), GetTableDataFromPageArea (lines 508–541).
🤖 Prompt for AI Agents
In Ginger/GingerCore/GingerOCR/GingerOcrOperations.cs (remove the unused Tabula
using at line ~30, delete the table-extraction comment around line 422, and
remove the GetTableDataFromPageArea method at lines 508–541), remove the dead
Tabula-related code and then delete the Tabula NuGet/package reference from the
project files (update the .csproj and/or packages.config and
Directory.Packages.props as applicable). Ensure no other code references the
method or Tabula types, run a build to verify, and commit the removals.
| namespace GingerCoreTest.Misc | ||
| { | ||
| [TestClass] | ||
| [Ignore] |
There was a problem hiding this comment.
🧩 Analysis chain
Blocking: Don’t blanket‑disable OCR tests; re‑enable and adapt assertions for PaddleOCR.
Class‑level [Ignore] skips all OCR tests, removing coverage for a large, risky engine migration and contradicting the PR readiness checklist (“existing unit tests passing”). Update tests to be tolerant to OCR engine/string variability and keep them running in CI.
Apply this minimal change to re‑enable and still allow selective filtering:
- [Ignore]
+ [TestCategory("OCR")]Then, replace brittle exact string equality with normalized/containment assertions (OCR spacing/newlines differ across engines). Example helper and usage (outside the changed line):
using System.Text.RegularExpressions;
static string Norm(string s) => Regex.Replace(s ?? string.Empty, @"\s+", " ").Trim();
[TestMethod]
public void ReadAllTextImage()
{
string txtOutput = GingerOcrOperations.ReadTextFromImage(OcrImageAllTextFilePath);
StringAssert.Contains(Norm(txtOutput).ToLowerInvariant(), "microsoft teams meeting");
}
[TestMethod]
public void ReadTextBetweenLabelsImage()
{
string err = string.Empty;
string txtOutput = GingerOcrOperations.ReadTextFromImageBetweenStrings(OcrImageFilePath, "Version", "Steps", ref err);
StringAssert.Contains(Norm(txtOutput), "4.1");
}Apply similar normalization/Contains checks to the remaining tests (assert key tokens like “107W0000”, “MOHD AZHARI BIN MAD ATARI”, and “70020776”, not fragile punctuation/newlines).
Run to ensure no other test suites are silently ignored:
🏁 Script executed:
#!/bin/bash
# List all ignored tests across the repo (excluding build artifacts)
fd -t f -E .git -E bin -E obj | xargs rg -nP '\[\s*Ignore(\s*\(|\s*\])' -HLength of output: 42419
Blocking: Re-enable OCR tests — remove class-level [Ignore] and make assertions tolerant to PaddleOCR
Class-level [Ignore] at Ginger/GingerCoreTest/Misc/OcrTest.cs:27 disables all OCR coverage; replace it and update assertions to tolerate engine-driven spacing/newline/punctuation differences.
Location: Ginger/GingerCoreTest/Misc/OcrTest.cs:27
- [Ignore]
+ [TestCategory("OCR")]Replace brittle exact-equality asserts with whitespace‑normalized, case‑insensitive containment. Example:
using System.Text.RegularExpressions;
static string Norm(string s) => Regex.Replace(s ?? string.Empty, @"\s+", " ").Trim();
[TestMethod]
public void ReadAllTextImage()
{
var txt = Norm(GingerOcrOperations.ReadTextFromImage(OcrImageAllTextFilePath)).ToLowerInvariant();
StringAssert.Contains(txt, "microsoft teams meeting");
}Apply the same normalization/Contains checks to the other OCR tests — assert key tokens (e.g. "107W0000", "MOHD AZHARI BIN MAD ATARI", "70020776") instead of exact punctuation/newlines.
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Improvements
Chores
Tests