Skip to content

fix(connector_sdk) : Added a note in the README.md for the customer on how to resolve the error - #67

Merged
fivetran-JenasVimal merged 14 commits into
mainfrom
ibm-informix-improvement
Sep 10, 2026
Merged

fix(connector_sdk) : Added a note in the README.md for the customer on how to resolve the error #67
fivetran-JenasVimal merged 14 commits into
mainfrom
ibm-informix-improvement

Conversation

@fivetran-JenasVimal

@fivetran-JenasVimal fivetran-JenasVimal commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Description

This PR improves the Informix  ibm_db  example with clearer DRDA guidance and updates the connector.py logic to match the current  internal-sales  Informix test instance.

Why this change

 ibm_db  connectivity to Informix can require the DRDA listener path depending on deployment. We saw repeated  SQL30081N  failures when listener/port assumptions were unclear.
Also, the sample table metadata in this environment differs from the original defaults ( id  /  created_at ).

What changed

README ( ibm_informix/ibm_informix_using_ibm_db/README.md )
• Added a concise troubleshooting note for  SQL30081N .
• Clarified normal listener vs DRDA listener ( onsoctcp  vs  drsoctcp ).
• Linked the important config note to the troubleshooting section.

Connector code ( ibm_informix/ibm_informix_using_ibm_db/connector.py )
• Updated incremental query column in  update() :
•  created_at  →  created 
• Updated state cursor handling:
• from  data["created_at"]  to  data.get("created")  flow

Validation

image

Checklist

Check only the items that apply:

  • Connector code change: I ran fivetran debug and attached sanitized proof of a successful run.
  • New connector: I added a connector-level README.md with the relevant template sections and listed the connector in the root README.md.
  • All contributions: I confirmed that the change and its validation evidence contain no credentials, personal data, customer information, or other sensitive values.

See CONTRIBUTING.md for the complete contribution requirements.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The updated connector code needs required SDK example structure fixes (validate_configuration/docstrings) and safer SQL execution (bound parameter) before it’s ready to merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the IBM Informix (ibm_db) Connector SDK example to provide additional connection troubleshooting guidance (DRDA vs native listener) and to align the incremental sync cursor column with the current Informix test instance.

Changes:

  • Added an SQL30081N troubleshooting note and a DRDA listener explanation to the example README.
  • Updated connector schema metadata to use tabid as the primary key (with an explicit column type).
  • Updated the incremental sync logic to query and advance state based on the created column instead of created_at.
File summaries
File Description
ibm_informix/ibm_informix_using_ibm_db/README.md Adds SQL30081N/DRDA troubleshooting guidance near configuration and in the Troubleshooting section.
ibm_informix/ibm_informix_using_ibm_db/connector.py Updates schema primary key/columns and switches incremental cursor handling from created_at to created.
Review details

Suppressed comments (1)

ibm_informix/ibm_informix_using_ibm_db/connector.py:140

  • The incremental query builds SQL via string interpolation for last_created; use a bound parameter to avoid quoting issues and reduce SQL-injection risk.
    sql = f"SELECT * FROM {table_name} WHERE created > '{last_created}'"
    # Execute the SQL query
    stmt = ibm_db.exec_immediate(conn, sql)
    # Fetch the first record from the result set
    # The ibm_db.fetch_assoc method fetches the next row from the result set as a dictionary
    data = ibm_db.fetch_assoc(stmt)
  • Files reviewed: 2/2 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ibm_informix/ibm_informix_using_ibm_db/connector.py
Comment thread ibm_informix/ibm_informix_using_ibm_db/connector.py
Comment thread ibm_informix/ibm_informix_using_ibm_db/README.md Outdated
Copilot AI review requested due to automatic review settings September 9, 2026 06:49
@fivetran-JenasVimal

Copy link
Copy Markdown
Contributor Author
image Output of latest commit

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The updated connector still interpolates unvalidated identifiers/state into SQL and the README has remaining content that is now inconsistent with the connector’s updated schema/cursor fields.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

ibm_informix/ibm_informix_using_ibm_db/connector.py:169

  • The incremental SQL query interpolates the state cursor (last_created) directly into the statement. Even though state is internal, using a parameterized query avoids quoting/injection issues and is the safer default for examples.
    sql = f"SELECT * FROM {table_name} WHERE created > '{last_created}'"
    # Execute the SQL query
    stmt = ibm_db.exec_immediate(conn, sql)
    # Fetch the first record from the result set
    # The ibm_db.fetch_assoc method fetches the next row from the result set as a dictionary
    data = ibm_db.fetch_assoc(stmt)

ibm_informix/ibm_informix_using_ibm_db/README.md:52

  • The new SQL30081N troubleshooting note is helpful, but the README’s existing Data handling/schema example is now out of sync with the connector changes in this PR (it still references created_at and primary_key id at README.md:75-83, while connector.py uses created and primary_key tabid). Please update the Data handling section/schema snippet to match the current example.

ibm_informix/ibm_informix_using_ibm_db/connector.py:39

  • validate_configuration() only checks presence/emptiness. Because table_name is interpolated directly into SQL, this should also validate table_name is a safe SQL identifier (or schema-qualified identifier) to avoid SQL injection, and validate/normalize port to an integer early so connection string building is consistent.
    required_keys = ["hostname", "port", "database", "username", "password", "table_name"]
    for key in required_keys:
        value = configuration.get(key)
        if value is None or str(value).strip() == "":
            raise ValueError(f"Missing required configuration key: {key}")
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 9, 2026 07:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are documentation and example-quality inconsistencies (misleading log message, non-standard required upsert comment template, and README schema example drift) that should be corrected before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

ibm_informix/ibm_informix_using_ibm_db/connector.py:230

  • The log.info message says "products table", but this connector is reading from table_name and writing to the sample_table destination. This message is misleading when debugging runs.
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread ibm_informix/ibm_informix_using_ibm_db/README.md
Comment thread ibm_informix/ibm_informix_using_ibm_db/connector.py Outdated
Copilot AI review requested due to automatic review settings September 9, 2026 08:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The incremental query can miss same-timestamp rows and the README still lacks the required “Tables created” section.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

ibm_informix/ibm_informix_using_ibm_db/connector.py:229

  • Log message references the "products" table, but this example upserts into sample_table. This makes debugging confusing when verifying sync results.
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread ibm_informix/ibm_informix_using_ibm_db/connector.py
Comment thread ibm_informix/ibm_informix_using_ibm_db/README.md
Copilot AI review requested due to automatic review settings September 9, 2026 09:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The updated connector still needs periodic checkpointing during the fetch loop and a couple of example/logging consistency fixes before it’s safe and consistent to publish.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

ibm_informix/ibm_informix_using_ibm_db/connector.py:219

  • State is only checkpointed once at the end of the sync; for large tables this risks losing a lot of progress on interruption and can increase rerun time. Checkpoint periodically during the fetch loop after successful upserts (e.g., every N records).
    ibm_informix/ibm_informix_using_ibm_db/connector.py:190
  • The first log statement in update() should follow the required format log.warning("Example: <CATEGORY> : <EXAMPLE_NAME>") so README and internal examples are consistently discoverable.
    ibm_informix/ibm_informix_using_ibm_db/connector.py:229
  • This log message refers to a "products" table, but the connector upserts into sample_table and reads from the configured table_name. This can confuse users when troubleshooting syncs.
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 9, 2026 16:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

There are a couple of small but concrete issues in connector.py (required example log format and a misleading log message) that should be corrected before approval.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

ibm_informix/ibm_informix_using_ibm_db/connector.py:199

  • The first log statement in update() should follow the required "Example: : <EXAMPLE_NAME>" format so examples are consistently categorized across the repo.

This issue also appears on line 238 of the same file.

ibm_informix/ibm_informix_using_ibm_db/connector.py:238

  • This log message refers to the "products" table, but this connector reads from the configured source table and upserts into destination table "sample_table". The current message is misleading when debugging runs.
    log.info("upserted all records from the products table")
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The connector’s DB connection path does not actually use the configured password (connection string ends with a hard-coded mask and ibm_db.connect is called with empty credentials), which prevents successful connections.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread ibm_informix/ibm_informix_using_ibm_db/connector.py
Comment thread ibm_informix/ibm_informix_using_ibm_db/connector.py Outdated
Copilot AI review requested due to automatic review settings September 10, 2026 11:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The connector’s DSN construction still appears to omit the configured password (uses a literal ******;), which will prevent successful connections even with valid configuration.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

ibm_informix/ibm_informix_using_ibm_db/README.md:86

  • The schema example JSON omits the created column, but the connector’s incremental logic depends on it (WHERE created > ? and state["last_created"]). Update the example schema snippet to include created with a datetime type so users don’t create a table definition that diverges from the connector’s behavior.

This issue also appears on line 98 of the same file.

ibm_informix/ibm_informix_using_ibm_db/connector.py:204

  • connect_to_db() currently builds a DSN that does not include the configured password: get_connection_string() reads configuration["password"] but the returned string ends with a literal "******;" (connector.py:123-135). As a result, ibm_db.connect(conn_str, "", "") will fail authentication even when a valid password is provided in configuration.json.
    # The date format of the created column in the database is "YYYY-MM-DD HH:MM:SS"
    # Please ensure that while handling the datetime, you are using the correct format for the columns.
    last_created = state.get("last_created", "1990-01-01 00:00:00")

ibm_informix/ibm_informix_using_ibm_db/connector.py:110

  • schema() defines only the primary key column (tabid) in the columns map, but update() filters and checkpoints on a created column and upserts full rows from SELECT *. If schema columns are enforced, the missing created field can lead to missing data in the destination table (or upsert failures). Consider explicitly including created with the appropriate datetime type.
    ]


# This method is used to create a connection string for IBM Informix database.

ibm_informix/ibm_informix_using_ibm_db/README.md:102

  • The Tables created section lists only tabid, but the connector’s incremental cursor and upserts rely on a created field. To keep the documentation consistent with the connector behavior, include created in the listed columns (and indicate its datetime type).
The connector creates the following destination table (refer to the `schema()` function in `connector.py`):

| Table          | Primary key | Columns |
|----------------|---|---|
| `sample_table` | `tabid` | `tabid` (`INT`) |
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 10, 2026 11:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The current connector.py changes still allow a connection path that will fail at runtime and the incremental predicate can skip records when timestamps collide.

Review details

Suppressed comments (2)

ibm_informix/ibm_informix_using_ibm_db/connector.py:212

  • The incremental filter uses created > ?, which can permanently skip rows when multiple records share the same created timestamp (common with low-precision timestamps). Using >= is safer because upsert is idempotent and prevents data loss across runs.
    # Use a parameter placeholder for the incremental cursor to avoid manual quote escaping in SQL text.
    sql = f"SELECT * FROM {table_name} WHERE created > ?"
    # Prepare the SQL template once, then bind data values separately.
    stmt = ibm_db.prepare(conn, sql)
    # Bind the current cursor value as a parameter so the driver handles quoting and typing safely.

ibm_informix/ibm_informix_using_ibm_db/connector.py:200

  • update() calls connect_to_db(), but get_connection_string() currently returns a DSN ending with a literal "******;" rather than a valid password/keyword. That makes the connection string invalid and will cause ibm_db.connect() to fail even when configuration includes a password.
    # Connect to the IBM Informix database
    conn = connect_to_db(configuration)
    table_name = configuration.get("table_name")
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@fivetran-JenasVimal

Copy link
Copy Markdown
Contributor Author

update() calls connect_to_db(), but get_connection_string() currently returns a DSN ending with a literal "******;" rather than a valid password/keyword. That makes the connection string invalid and will cause ibm_db.connect() to fail even when configuration includes a password.
Resolved — this is a false positive from masked output.
 get_connection_string()  uses  PWD={password};  in the DSN, not a literal  ******; . The  ******  seen in some logs/views is redaction of credential-like text.  ibm_db.connect(conn_str, "", "")  is intentional because credentials are supplied in  conn_str  via  UID  and  PWD .

Comment thread ibm_informix/ibm_informix_using_ibm_db/README.md Outdated
Comment thread ibm_informix/ibm_informix_using_ibm_db/README.md Outdated
Comment thread ibm_informix/ibm_informix_using_ibm_db/README.md Outdated
Comment thread ibm_informix/ibm_informix_using_ibm_db/README.md Outdated
Co-authored-by: Sahil Khirwal <sahil.khirwal@fivetran.com>
Copilot AI review requested due to automatic review settings September 10, 2026 16:49
fivetran-JenasVimal and others added 2 commits September 10, 2026 22:20
Co-authored-by: Sahil Khirwal <sahil.khirwal@fivetran.com>
Co-authored-by: Sahil Khirwal <sahil.khirwal@fivetran.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The updated incremental query uses a strict > predicate, which can miss records inserted later with the same created timestamp as the last checkpoint.

Review details

Suppressed comments (1)

ibm_informix/ibm_informix_using_ibm_db/connector.py:210

  • Using a strict created > ? filter can miss rows inserted later with the same created timestamp as the last checkpoint (common when the column has second-level precision). Using >= avoids data loss; duplicates are safe because the connector uses upsert with a primary key.
    # Use a parameter placeholder for the incremental cursor to avoid manual quote escaping in SQL text.
    sql = f"SELECT * FROM {table_name} WHERE created > ?"
    # Prepare the SQL template once, then bind data values separately.
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 10, 2026 16:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The connector still appears to omit the configured password in the generated DSN (a literal ******; is present in get_connection_string()), which would prevent real connections and should be corrected/verified before merge.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

ibm_informix/ibm_informix_using_ibm_db/connector.py:220

  • The loop can process an arbitrarily large table but only checkpoints once at the end, which can cause long re-syncs and lost progress if the sync is interrupted. Consider checkpointing at a fixed interval inside the fetch loop (while still keeping the final checkpoint).
    ibm_informix/ibm_informix_using_ibm_db/README.md:102
  • The Markdown table separator row uses uneven column markers (|----------------|---|---|), which can render inconsistently across Markdown processors. Using the standard | --- | --- | --- | separator improves portability.
    ibm_informix/ibm_informix_using_ibm_db/connector.py:270
  • The local testing block loads configuration.json but then passes the config dict into connector.debug(...); the repo template uses connector.debug() after loading the file. Aligning with the standard template keeps examples consistent.
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment thread ibm_informix/ibm_informix_using_ibm_db/README.md Outdated
Copilot AI review requested due to automatic review settings September 10, 2026 17:07

@5tran-alexil 5tran-alexil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

README LGTM

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The connector still does not include the configured password in the DSN (uses a literal ******;), which will prevent successful authentication despite validating password as required.

Review details

Suppressed comments (5)

Previously missed (2) — in code that hasn't changed since the last review.

ibm_informix/ibm_informix_using_ibm_db/connector.py:26

  • To avoid a magic number when adding periodic checkpointing, define a module-level __CHECKPOINT_INTERVAL constant near the imports and reuse it in update().

This issue also appears on line 218 of the same file.
ibm_informix/ibm_informix_using_ibm_db/connector.py:195

  • The first log line in update() is required to follow the "Example: : <EXAMPLE_NAME>" format; the current message uses a dash instead of the separator colon.

ibm_informix/ibm_informix_using_ibm_db/connector.py:224

  • The code comments recommend checkpointing regularly for large datasets, but the implementation only checkpoints once at the end; this increases rerun time after failures. Consider checkpointing every N records during the fetch loop.
    data = ibm_db.fetch_assoc(stmt)
    # Iterate over the result set and upsert each record until there are no more records
    while data:
        # The 'upsert' operation is used to insert or update data in the destination table.
        # The first argument is the name of the destination table.
        # The second argument is a dictionary containing the record to be upserted.
        op.upsert(table="sample_table", data=data)

ibm_informix/ibm_informix_using_ibm_db/connector.py:210

  • Using a strict '>' cursor can skip rows that share the same created timestamp as the last processed record (common when timestamps have low precision). Using '>=' is safer here since upsert is idempotent on the primary key.
    # Use a parameter placeholder for the incremental cursor to avoid manual quote escaping in SQL text.
    sql = f"SELECT * FROM {table_name} WHERE created > ?"
    # Prepare the SQL template once, then bind data values separately.

ibm_informix/ibm_informix_using_ibm_db/connector.py:63

  • validate_configuration() requires a password, but get_connection_string() currently does not include it in the DSN (it ends with a literal "******;"), so authentication will fail even with valid configuration.
    required_keys = [
        "hostname",
        "port",
        "database",
        "username",
        "password",
        "table_name",
    ]
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@fivetran-JenasVimal
fivetran-JenasVimal merged commit fd1a980 into main Sep 10, 2026
4 checks passed
@fivetran-JenasVimal
fivetran-JenasVimal deleted the ibm-informix-improvement branch September 10, 2026 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants