Skip to content

Conversation

@kbighorse
Copy link
Contributor

@kbighorse kbighorse commented Jan 22, 2026

Summary

Enforces parent-child integrity for NMA legacy tables and refactors them from UUID to Integer primary keys for consistency with the rest of the schema.

Fixes #363

Changes

1. Integer PK Refactoring

  • Added id (Integer, autoincrement) as primary key to 8 NMA tables
  • Renamed existing UUID columns with nma_ prefix for audit/traceability
  • Standardized FK naming to chemistry_sample_info_id (Integer) pattern

2. Tables Modified

Table New PK Legacy UUID Column
NMA_Chemistry_SampleInfo id nma_sample_pt_id
NMA_FieldParameters id nma_global_id
NMA_MajorChemistry id nma_global_id
NMA_MinorTraceChemistry id nma_global_id
NMA_Radionuclides id nma_global_id
NMA_HydraulicsData id nma_global_id
NMA_Stratigraphy id nma_global_id
NMA_AssociatedData id nma_assoc_id

3. FK Enforcement

  • All NMA child tables require a parent Thing (thing_id NOT NULL)
  • Chemistry child tables require parent sample info (chemistry_sample_info_id NOT NULL)
  • Cascade deletes enabled: deleting a Thing removes all related NMA records

4. Code Updates

  • Models (db/nma_legacy.py): Updated column definitions and relationships
  • Transfers (transfers/*.py): Updated column mappings for new schema
  • Admin views (admin/views/*.py): Updated pk_attr and pk_type
  • Tests: Updated for Integer PKs and correct DB column name access

Orphan Records Discovered

The FK enforcement successfully prevented orphan records from being transferred. The following records exist in the legacy SQL Server database but have no valid parent record:

Table Valid Records Orphan Records Prevented
HydraulicsData 0 288
ChemistrySampleInfo 1,622 9,820
MajorChemistry 16,532 62,046
Radionuclides 12 351
MinorTraceChemistry 31,721 76,640
FieldParameters 2,899 11,035

Total orphan records prevented: ~160,180

Example Orphan Subtree

┌─────────────────────────────────────────────────────────────────────┐
│ LEVEL 0: Thing (MISSING - causes orphan cascade)                    │
├─────────────────────────────────────────────────────────────────────┤
│ Expected name: ar-1001                                              │
│ Status: NOT FOUND in WellData.csv                                   │
│ Reason: Well was never registered in the source database           │
└─────────────────────────────────────────────────────────────────────┘
          │
          │ (FK: thing_id → Thing.id)
          ▼
┌─────────────────────────────────────────────────────────────────────┐
│ LEVEL 1: ChemistrySampleInfo (ORPHAN)                               │
├─────────────────────────────────────────────────────────────────────┤
│ SamplePointID: AR-1001B                                             │
│ SamplePtID: FA79D688-8BCB-B64F-8E2D-409324D0DD8C                    │
│ CollectionDate: 2018-10-02                                          │
│                                                                     │
│ Status: FILTERED OUT - No parent Thing exists                       │
└─────────────────────────────────────────────────────────────────────┘
          │
          │ (FK: chemistry_sample_info_id → ChemistrySampleInfo.id)
          ▼
┌─────────────────────────────────────────────────────────────────────┐
│ LEVEL 2: MinorTraceChemistry (62 ORPHAN records)                    │
├─────────────────────────────────────────────────────────────────────┤
│   • Ba(total) = 0.001                                               │
│   • Ti(total) = 0.001                                               │
│   • Pb = 0.0005                                                     │
│   ... and 59 more records                                           │
│                                                                     │
│ Status: ALL FILTERED OUT - Parent ChemistrySampleInfo is orphan     │
└─────────────────────────────────────────────────────────────────────┘

Root cause: No Well record exists in the source database for PointID "AR-1001"
Impact: 1 ChemistrySampleInfo + 62 chemistry results = 63 orphan records prevented

The high number of chemistry orphans cascades from ChemistrySampleInfo - when sample info records are filtered out for lacking a parent Thing, their child chemistry records become orphans too.

Test Plan

  • All 436 tests passing
  • Transfer scripts run successfully against dev database
  • Cascade deletes verified in integration tests
  • Orphan records correctly filtered during transfer

🤖 Generated with Claude Code

Defines business requirements for:
- Wells storing legacy NM_Aquifer identifiers (WellID, LocationID)
- Related records (chemistry, hydraulics, stratigraphy, etc.) requiring a well
- Cascade delete behavior when wells are removed

Addresses #363

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 22, 2026 19:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a comprehensive feature specification that defines business requirements for well data relationships in the NMBGMR system, ensuring parent-child integrity between wells and their associated data records.

Changes:

  • Defines requirements for wells to store legacy identifiers (WellID and LocationID) from NM_Aquifer
  • Specifies that all well-related records (chemistry, hydraulics, stratigraphy, radionuclides, associated data, soil/rock) must have a parent well
  • Establishes cascade delete behavior to prevent orphaned records when wells are deleted

kbighorse and others added 2 commits January 22, 2026 11:34
Adds scenario for navigating from a well to its related records
through ORM relationships.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 22, 2026 19:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

kbighorse and others added 5 commits January 26, 2026 09:56
Add integration and unit tests for well data relationships feature:

- Integration tests (test_well_data_relationships.py):
  - Wells store legacy identifiers (nma_pk_welldata, nma_pk_location)
  - Related records require a well (thing_id cannot be None)
  - Relationship navigation from Thing to NMA legacy models
  - Cascade delete behavior

- Unit tests added to existing files:
  - test_thing.py: Thing column and relationship assertions
  - test_hydraulics_data_legacy.py: validator and back_populates
  - test_associated_data_legacy.py: validator and back_populates
  - test_soil_rock_results_legacy.py: validator and back_populates
  - test_radionuclides_legacy.py: FK cascade and back_populates
  - test_stratigraphy_legacy.py (new): validator and back_populates

These tests are expected to fail until the model changes are implemented.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update NMA_AssociatedData and NMA_Soil_Rock_Results minimal creation
tests to include a thing_id, preparing for NOT NULL constraint.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
db/thing.py:
- Add nma_pk_location column for legacy NM_Aquifer LocationID
- Add relationship collections: hydraulics_data, radionuclides,
  associated_data, soil_rock_results
- Configure cascade="all, delete-orphan" on all NMA relationships

db/nma_legacy.py:
- Add @validates("thing_id") to NMA_HydraulicsData, NMA_Stratigraphy,
  NMA_AssociatedData, NMA_Soil_Rock_Results
- Add back_populates to NMA_HydraulicsData, NMA_AssociatedData,
  NMA_Soil_Rock_Results, NMA_Radionuclides
- Change thing_id to NOT NULL on NMA_AssociatedData, NMA_Soil_Rock_Results

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add nma_pk_location column to thing table
- Delete orphan records from NMA_AssociatedData and NMA_Soil_Rock_Results
- Make thing_id NOT NULL on NMA_AssociatedData and NMA_Soil_Rock_Results

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 26, 2026 20:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

SQLAlchemy-continuum creates a thing_version table that mirrors
the thing table structure. The migration must add the new column
to both tables for versioning to work correctly.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 26, 2026 23:41
kbighorse and others added 2 commits January 26, 2026 15:41
- Fix import names in BDD step file (use NMA_ prefix)
- Fix radionuclide tests to create chemistry sample first
  (satisfies sample_pt_id FK constraint)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.

kbighorse and others added 2 commits January 26, 2026 15:45
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Restore POSTGRES_DB and POSTGRES_PORT settings that were accidentally
removed in commit 62ecda1 during the NMA_ prefix refactoring.

Without these settings, tests would connect to ocotilloapi_dev instead
of ocotilloapi_test because load_dotenv(override=True) would overwrite
the POSTGRES_DB set by pytest_configure().

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 28, 2026 01:58
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

kbighorse and others added 5 commits January 27, 2026 17:59
- Add test_top/test_bottom to NMA_HydraulicsData test fixtures
- Add global_id to NMA_Radionuclides test fixtures
- Add session.expire_all() before cascade delete assertions to clear
  SQLAlchemy's identity map cache (passive_deletes relies on DB cascade)
- Fix point_id values to respect max 10 char constraint

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update all NMA legacy models to use Integer autoincrement primary keys
instead of UUID PKs. Legacy columns are renamed with nma_ prefix for
audit/traceability.

Changes per table:
- NMA_HydraulicsData: id (Integer PK), nma_global_id, nma_well_id, nma_point_id, nma_object_id
- NMA_Stratigraphy: id (Integer PK), nma_global_id, nma_well_id, nma_point_id, nma_object_id
- NMA_Chemistry_SampleInfo: id (Integer PK), nma_sample_pt_id, nma_sample_point_id, nma_wclab_id, nma_location_id, nma_object_id
- NMA_AssociatedData: id (Integer PK), nma_assoc_id, nma_location_id, nma_point_id, nma_object_id
- NMA_Radionuclides: id (Integer PK), nma_global_id, chemistry_sample_info_id (Integer FK), nma_sample_pt_id, nma_sample_point_id, nma_object_id, nma_wclab_id
- NMA_MinorTraceChemistry: id (Integer PK), nma_global_id, chemistry_sample_info_id (Integer FK), nma_chemistry_sample_info_uuid
- NMA_MajorChemistry: id (Integer PK), nma_global_id, chemistry_sample_info_id (Integer FK), nma_sample_pt_id, nma_sample_point_id, nma_object_id, nma_wclab_id
- NMA_FieldParameters: id (Integer PK), nma_global_id, chemistry_sample_info_id (Integer FK), nma_sample_pt_id, nma_sample_point_id, nma_object_id, nma_wclab_id
- NMA_Soil_Rock_Results: nma_point_id (rename only, already had Integer PK)

Chemistry chain children now use Integer FK (chemistry_sample_info_id)
pointing to NMA_Chemistry_SampleInfo.id instead of UUID FK.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update all transfer scripts to use nma_ prefixed column names and
Integer FK relationships for chemistry chain.

Changes:
- chemistry_sampleinfo.py: Map to nma_sample_pt_id, nma_sample_point_id, nma_wclab_id, nma_location_id, nma_object_id
- minor_trace_chemistry_transfer.py: Use Integer FK via chemistry_sample_info_id lookup, store legacy UUID in nma_chemistry_sample_info_uuid
- radionuclides.py: Use Integer FK via chemistry_sample_info_id lookup, map to nma_* columns
- field_parameters_transfer.py: Use Integer FK via chemistry_sample_info_id lookup, map to nma_* columns
- major_chemistry.py: Use Integer FK via chemistry_sample_info_id lookup, map to nma_* columns
- stratigraphy_legacy.py: Map to nma_global_id, nma_well_id, nma_point_id, nma_object_id
- associated_data.py: Map to nma_assoc_id, nma_location_id, nma_point_id, nma_object_id
- hydraulicsdata.py: Map to nma_global_id, nma_well_id, nma_point_id, nma_object_id
- soil_rock_results.py: Map to nma_point_id

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update all NMA admin views to use Integer primary keys and nma_ prefixed
field names for display.

Changes to all views:
- Set pk_attr = "id" and pk_type = int
- Update list_fields, fields, sortable_fields, searchable_fields with nma_ prefix
- Update field_labels with "(Legacy)" suffix for audit columns

Files updated:
- chemistry_sampleinfo.py
- hydraulicsdata.py
- stratigraphy.py
- radionuclides.py
- minor_trace_chemistry.py
- field_parameters.py
- soil_rock_results.py

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update all unit tests to use Integer PK (id) and nma_ prefixed columns.
Add new tests for Integer PK validation and unique constraints.

Changes:
- Replace global_id, sample_pt_id, etc. with nma_global_id, nma_sample_pt_id
- Replace UUID PK assertions with Integer PK assertions
- Use chemistry_sample_info_id (Integer FK) instead of sample_pt_id (UUID FK)
- Add tests for Integer PK column type and unique constraints
- Update admin view tests for new field names and labels

Files updated:
- test_stratigraphy_legacy.py
- test_associated_data_legacy.py
- test_radionuclides_legacy.py
- test_field_parameters_legacy.py
- test_major_chemistry_legacy.py
- test_chemistry_sampleinfo_legacy.py
- test_hydraulics_data_legacy.py
- test_soil_rock_results_legacy.py
- test_admin_minor_trace_chemistry.py

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
kbighorse and others added 3 commits January 28, 2026 01:28
Update integration and BDD tests to use Integer PK (id) and nma_
prefixed columns for all NMA legacy models.

Changes:
- Replace global_id, sample_pt_id, point_id, etc. with nma_ prefixed versions
- Use chemistry_sample_info_id (Integer FK) for radionuclides relationship
- Update cascade delete tests to use Integer PK for record lookup
- Update relationship navigation tests to check nma_ prefixed columns

Files updated:
- tests/integration/test_well_data_relationships.py
- tests/features/steps/well-data-relationships.py

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add migration to refactor NMA tables from UUID to Integer primary keys:
- Add `id` (Integer PK with IDENTITY) to 8 NMA tables
- Rename UUID columns with `nma_` prefix for audit/traceability
- Convert FK references from UUID to Integer
- Make `chemistry_sample_info_id` NOT NULL for chemistry child tables

Also fixes alembic/env.py to handle None names for unnamed constraints,
and updates test files to use correct DB column names via bracket notation
(e.g., `__table__.c["nma_GlobalID"]` instead of `__table__.c.nma_global_id`).

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 28, 2026 10:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

kbighorse and others added 2 commits January 28, 2026 02:28
Resolved conflicts:
- admin/views/field_parameters.py: Combined Integer PK attrs with method-based permissions
- db/nma_legacy.py: Kept Integer PK schema for NMA_Stratigraphy with nma_ prefixed columns

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Post-merge fixes:
- admin/views/associated_data.py: Update to use nma_ prefixed columns (Integer PK)
- admin/views/major_chemistry.py: Update to use nma_ prefixed columns (Integer PK)
- tests/test_stratigraphy_legacy.py: Add required strat_top/strat_bottom fields
- tests/integration/test_well_data_relationships.py: Add required strat_top/strat_bottom fields

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@kbighorse
Copy link
Contributor Author

@chasetmartin the one major change is replacing UUID primary keys with integers. We could keep the UUIDs if there's good reason to, but this fits the need IMO.

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.

WellData and Location (anything that becomes a Thing) children should have enforced FK associations with orphan prevention

2 participants