feat(contrib): harbour-orm — ActiveRecord ORM for Harbour over the ACE ABI#29
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces harbour-orm, an ActiveRecord-style ORM for Harbour, alongside core engine updates to support transaction rollbacks and multi-tag CDX index handling. It also adds openmonitor, a Rust-based TUI and web dashboard for monitoring openads_serverd. Feedback on the ORM includes addressing potential null pointer dereferences in the C glue, optimizing sequential table scans to index-based seeks, preventing DateTime truncation during SQL compilation, implementing a destructor to avoid resource leaks, and retrieving auto-incremented primary keys after record creation. For openmonitor, recommendations include enforcing an upper bound on frame lengths to prevent OOM panics and offloading blocking network I/O to a background thread to keep the TUI responsive.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| UNSIGNED32 ulRc = AdsGetField( hCursor, ( UNSIGNED8 * ) hb_parc( 2 ), | ||
| ( UNSIGNED8 * ) pBuf, &ulLen, 0 ); |
There was a problem hiding this comment.
hb_parc( 2 ) can return NULL if the second parameter is omitted or is not a string. Passing NULL directly to AdsGetField will cause a crash or undefined behavior. It should be checked before use.
const char * szName = hb_parc( 2 );
UNSIGNED32 ulRc = 1;
if( szName )
ulRc = AdsGetField( hCursor, ( UNSIGNED8 * ) szName,
( UNSIGNED8 * ) pBuf, &ulLen, 0 );
| DO WHILE ! hbo_Eof( nTbl ) | ||
| IF ! hbo_IsDeleted( nTbl ) .AND. AllTrim( hbo_Field( nTbl, cPk ) ) == cTarget | ||
| hRow := NavReadRow( nTbl ) | ||
| EXIT | ||
| ENDIF | ||
| hbo_Skip( nTbl, 1 ) | ||
| ENDDO |
There was a problem hiding this comment.
Performing a sequential full table scan (DO WHILE ! hbo_Eof( nTbl )) for single-record operations (NavFind, NavUpdate, NavDelete) is highly inefficient (O(N) complexity). As the table grows, this will cause severe performance degradation. Consider using index-based seeks (e.g., AdsSeek or equivalent index lookup) to find records by primary key in O(log N) time.
| .context("read frame header")?; | ||
| let len = u32::from_be_bytes([header[0], header[1], header[2], header[3]]) as usize; | ||
| let opcode = header[4]; |
There was a problem hiding this comment.
The frame length len is read directly from the TCP stream and used to allocate the payload vector without any upper bound validation. A corrupted or malicious packet with a large length header could trigger an Out-Of-Memory (OOM) panic and crash the monitor. An upper limit (e.g., 10 MB) should be enforced before allocation.
let len = u32::from_be_bytes([header[0], header[1], header[2], header[3]]) as usize;
if len > 10 * 1024 * 1024 {
return Err(anyhow!("frame length {} exceeds maximum limit of 10MB", len));
}
let opcode = header[4];
let mut payload = vec![0u8; len];| CASE HB_ISDATE( xVal ) | ||
| RETURN "'" + iif( Empty( xVal ), "", DToC4( xVal ) ) + "'" |
There was a problem hiding this comment.
In Harbour, HB_ISDATE() returns true for both Date and DateTime values. However, DToC4 only formats the date portion (YYYY-MM-DD), which means any DateTime value will have its time portion silently truncated and lost when quoted. Adding a specific check for HB_ISDATETIME() before HB_ISDATE() will preserve the time portion.
CASE HB_ISDATETIME( xVal )
RETURN "'" + iif( Empty( xVal ), "", hb_TToC( xVal, "YYYY-MM-DD", "HH:MM:SS" ) ) + "'"
CASE HB_ISDATE( xVal )
RETURN "'" + iif( Empty( xVal ), "", DToC4( xVal ) ) + "'"
| fn refresh(&mut self, cfg: &MonitorConfig) { | ||
| self.inner.refresh(cfg); | ||
| self.last_refresh = Instant::now(); | ||
| } |
There was a problem hiding this comment.
The refresh method performs blocking network I/O (both TCP connection attempts and HTTP requests) directly on the TUI's main thread. If the monitored server is slow, firewalled, or offline, the entire user interface will freeze and become unresponsive to user input (such as pressing q to quit) for several seconds. Consider performing network polling on a background thread and updating the TUI state asynchronously.
| CREATE CLASS TORMConnection | ||
| DATA nConn INIT 0 | ||
| DATA cUri INIT "" | ||
| DATA lNav INIT .F. | ||
| METHOD New( cUri ) CONSTRUCTOR | ||
| METHOD IsOpen() INLINE ::nConn != 0 | ||
| METHOD IsNavigational() INLINE ::lNav | ||
| METHOD Execute( cSql ) | ||
| METHOD Query( cSql ) | ||
| METHOD NavInsert( cTable, hValues ) | ||
| METHOD NavFind( cTable, cPk, xId ) | ||
| METHOD NavUpdate( cTable, cPk, xId, hValues ) | ||
| METHOD NavDelete( cTable, cPk, xId ) | ||
| METHOD Close() | ||
| METHOD LastError() INLINE hbo_LastErr() | ||
| END CLASS |
There was a problem hiding this comment.
The TORMConnection class does not define a destructor. If a connection object goes out of scope or is garbage collected without an explicit call to Close(), the underlying connection handle ::nConn will be leaked on the server/engine side. Defining a destructor that calls Close() ensures resources are cleaned up automatically.
CREATE CLASS TORMConnection
DATA nConn INIT 0
DATA cUri INIT ""
DATA lNav INIT .F.
METHOD New( cUri ) CONSTRUCTOR
METHOD IsOpen() INLINE ::nConn != 0
METHOD IsNavigational() INLINE ::lNav
METHOD Execute( cSql )
METHOD Query( cSql )
METHOD NavInsert( cTable, hValues )
METHOD NavFind( cTable, cPk, xId )
METHOD NavUpdate( cTable, cPk, xId, hValues )
METHOD NavDelete( cTable, cPk, xId )
METHOD Close()
METHOD LastError() INLINE hbo_LastErr()
DESTRUCTOR Destroy()
END CLASS
METHOD Destroy() CLASS TORMConnection
::Close()
RETURN Self
| METHOD Save() CLASS TORMModel | ||
| LOCAL oG, xId, hAst, cSql, lOk | ||
| IF ::oConn:IsNavigational() | ||
| IF ::lExists | ||
| lOk := ::oConn:NavUpdate( ::TableName(), ::PrimaryKey(), ; | ||
| ::Get( ::PrimaryKey() ), ::hAttrs ) | ||
| ELSE | ||
| lOk := ::oConn:NavInsert( ::TableName(), ::hAttrs ) | ||
| ENDIF | ||
| IF lOk | ||
| ::lExists := .T. | ||
| ENDIF | ||
| RETURN lOk | ||
| ENDIF |
There was a problem hiding this comment.
When creating a new record (where ::lExists is .F.), the database typically generates an auto-incremented primary key (e.g., id). However, Save() does not retrieve this generated ID and update the model's attributes (::hAttrs). As a result, the model instance will have a NIL primary key after creation, which prevents subsequent updates or deletions on the same instance unless it is manually re-fetched. Consider retrieving the last inserted ID (e.g., via an engine-specific API or query) and updating the primary key attribute upon successful insertion.
…E ABI Builds on #21 (SQL passthrough) and #28 (remote AdsSet* fixes). A small ActiveRecord-style ORM for Harbour that drives the OpenADS ACE ABI: models (Create/Find/Save/Delete), a fluent query builder, a dialect-agnostic grammar, relations, and schema/migrations. Backend is selected per deployment by the connection URI (sqlite://, a local DBF directory, tcp://, postgresql://, mariadb://, odbc://) — the same model code runs across all of them. Two execution paths: SQL for SQL-capable backends and a navigational table-cursor path (honors deletion; works where there is no SQL passthrough). Pure addition under contrib/ — not wired into the build, no CI impact. Verified out-of-tree: smoke 34/34 and a per-backend CRUD harness 14/14 (SQLite, local DBF, live wire server). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5173e0d to
a9006dc
Compare
Builds on #21 (SQL passthrough) and #28 (remote AdsSet* fixes).
Adds
contrib/harbour-orm/— a small ActiveRecord-style ORM for Harbour thatdrives the ACE ABI, so apps get models + a fluent query builder + relations +
migrations while picking the backend per deployment, by connection URI
(
sqlite://, a local DBF directory,tcp://,postgresql://,mariadb://,odbc://) — same model code across all of them.TORMModel(Create/Find/Save/Delete),TORMQuery(Where/OrderBy/Limit/Get),TORMGrammar(dialect-agnostic AST→SQL), relations (HasMany/HasOne/BelongsTo),TORMSchema/TORMBlueprint(migrations), and an ACE glue layer (hbo_ace.prg).table-cursor path for xBase/native and navigational-only backends (a
Findhonors
SET DELETED; writes work where there's no SQL passthrough).Pure addition under
contrib/— not wired into CMake, no build/CI impact.Verified out-of-tree:
tests/smoke.prg34/34 (SQL path) andtests/exhaust.prg14/14 per-backend CRUD across SQLite, a local DBF directory, and a live wire
server. Also mirrored at https://github.com/Admnwk/harbour-orm.