Repository navigation
Conversation
5046f6f to
9181c31
Compare
| ) | ||
|
|
||
| return constructor(res.fetchone()) | ||
| return self._construct_record(res.fetchone(), constructor) |
There was a problem hiding this comment.
This helper preserves consistent parsing across both supported access paths.
The investment managers already pass the database-level SchemaProfile to
investment constructors. However, get_record() and get_record_by_gid() also
accept model constructors directly. Calling one of those methods with
InvestmentHolding, InvestmentBuyTransaction, or InvestmentSellTransaction
must therefore pass the same profile.
Without this delegation, direct accessor calls would fall back to row-level
alias selection. That is unsafe for mixed schemas, where holdings and
transactions intentionally use different share columns. The same record could
then be interpreted differently depending on whether it was loaded through a
manager or directly through the accessor.
Non-investment constructors retain the original constructor(row) behavior.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9181c31626
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
could you please check if #40 solve the same issue? |
My reading is that #40 and #37 address related but different schema problems:
The key difference is that #40 resolves aliases row-by-row by selecting the first available column. That cannot safely distinguish those mixed investment layouts when both columns exist. #37 detects the database schema once, assigns aliases per consumer, and rejects ambiguous layouts. Therefore, #40 is related infrastructure, but it does not replace the consumer-specific investment handling in #37. I think #37 should be merged for this issue. Useful parts of #40, especially dynamic tag-table discovery, could be adapted separately while preserving #37’s schema-profile logic. |
https://github.com/ileodo/moneywiz-api/pull/40/changes#r3794767529 see this comment, for same model class and field, the column (aliases) selection will be deterministic. |
|
also the plan is to keep backward compatibility, then adding more alias options to each field's spec to support different version |
| self.number_of_shares = RDH.get_profile_decimal( | ||
| row, | ||
| schema_profile.transaction_number_of_shares_column | ||
| if schema_profile | ||
| else None, | ||
| "ZNUMBEROFSHARES1", | ||
| "ZNUMBEROFSHARES", | ||
| ) | ||
| self.price_per_share = RDH.get_profile_decimal( | ||
| row, | ||
| schema_profile.price_per_share_column if schema_profile else None, | ||
| "ZPRICEPERSHARE1", | ||
| "ZPRICEPERSHARE", | ||
| ) |
There was a problem hiding this comment.
this part can be achieved by adding more options to the fields spec https://github.com/ileodo/moneywiz-api/pull/40/changes#diff-8e3331851f50dcf4188715c82364729044ed90257c12a28557f7c7ed6ff4715dR20 after 40 is merged
There was a problem hiding this comment.
Agreed. I updated the investment mapping to use FieldSpec, following the approach introduced by #40. A field can now select its column through a profile_column selector, and SchemaMappedRow resolves it using the database-level SchemaProfile. Holdings and investment transactions declare their respective profile columns, while manager loads and direct accessor calls use the same generic mapping path. This makes the choice deterministic when multiple aliases are present and removes the investment-specific constructor branches. The tests cover mixed and unsuffixed mappings.
| investment_constructors = { | ||
| InvestmentBuyTransaction, | ||
| InvestmentSellTransaction, | ||
| InvestmentHolding, | ||
| } | ||
| if constructor in investment_constructors: | ||
| return constructor(row, schema_profile=self.schema_profile) |
There was a problem hiding this comment.
not super ideal that having special code path for some certain data model
There was a problem hiding this comment.
Agreed. I removed the investment-specific construction branches from DatabaseAccessor, TransactionManager, and InvestmentHoldingManager. The detected SchemaProfile now flows through the shared mapped_row() / SchemaMappedRow path, while the investment models declare their profile-specific columns in their FieldSpecs. Unsupported profiles still raise UnsupportedInvestmentSchemaError and propagate through RecordManager.load() rather than being silently treated as malformed rows. Tests cover both manager loading and direct accessor construction.
| if not self._table_exists("ZCATEGORYASSIGMENT"): | ||
| return transaction_map |
There was a problem hiding this comment.
would this ever happen in a real moneywiz database? or this is more for testing.
There was a problem hiding this comment.
I checked my current MoneyWiz database read-only. It does not have this exact partial-profile case: it has ZNUMBEROFSHARES, no ZNUMBEROFSHARES1, and both price columns. The detector resolves it as mixed-investment-columns, and the loaders read 100 holdings, 205 buy transactions, and 76 sell transactions.
The test creates a synthetic partial profile: the holding shares column is known, while the transaction shares and price columns are unresolved. It verifies that holdings can still load using their known column, while transactions fail closed when their required columns are unknown.
So I did not observe this exact case in my database. The test covers a schema edge case: an unresolved transaction mapping should not prevent reading holdings whose mapping is known. It does not add a test-only production mode or a user-selectable option.
| if not self._table_exists("Z_36TAGS"): | ||
| return transactions_to_tags |
There was a problem hiding this comment.
apparently the name for tag-transaction relation table can change. (it happen to me after migrating db to iCloud) #40 already change this to be dynamic.
e28b718 to
966dd60
Compare
Agreed. The PR keeps the investment fields compatible across the supported schema versions by selecting their columns through |
|
I pushed the code adjustments based on your comments and replied to each one. @ileodo, could you please do a final review? I hope this is now ready for merging. |
|
@ileodo how are you? I hope you can find some time to review this PR |
|
will take a look next week. |
|
Thank you!!
…On Sat, 3 Oct 2026 at 22:59, Tianwei Dong ***@***.***> wrote:
*ileodo* left a comment (ileodo/moneywiz-api#37)
<#37 (comment)>
will take a look next week.
—
Reply to this email directly, view it on GitHub
<#37?email_source=notifications&email_token=AAK4ZSFHCNMVMVQBMDDAIML5SFSD3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJXGM2DENZZGAZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5973427903>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAK4ZSHFQTZAELJDLCSUZBL5SFSD3AVCNFSNUABFKJSXA33TNF2G64TZHM3DIMRQGU3DSOJVHNEXG43VMU5TKMJTGUZDIMBUGEZ2C5QC>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Thank you for your work on this PR and for following up on my review. I understand your point better now: a MoneyWiz database can contain both investment column names. Choosing the first available alias can read a value for the wrong entity. Your use of database and entity-specific evidence addresses that risk. Rejecting a mapping when the evidence is unclear also makes sense. I refactored the schema code in #42 after our discussion. I borrowed your idea of resolving schema details once for the database and carrying that result through record creation. In #42, field definitions live in one SchemaProfile, inherit across record types, and use the same path for managers and direct accessor calls. I chose this structure so we can add mappings for more database versions without special constructor paths for investment models. #42 gives us that common path, but it does not yet solve the investment column issue you found. Could you update #37 against the current main and adapt your SchemaProfile detection to the SchemaProfile and SchemaProfileResolver in #42? Please keep separate column choices for holdings and buy/sell transactions, and reject cases where no safe choice is possible. Could you also bring over the _load_errors handling and its public load_errors view? I would like malformed rows to remain visible while a database-wide unsupported schema error still stops loading. I appreciate your careful code, tests, and patience. Your work showed me a gap in my earlier approach. |
Reject zero-rate transfer reconstructions as recoverable parse failures, cover manager recovery, and format schema compatibility tests.
Model holding and transaction share columns independently and preserve canonical stored amounts for rounded transfers.
- Removed the schema_profile.py file and integrated its functionality into the schema module. - Updated tests to utilize the new schema profile resolution approach. - Enhanced investment profile creation logic to accommodate custom field mappings. - Improved test coverage for ambiguous investment column scenarios and error handling. - Adjusted database setup in tests to ensure proper schema detection and validation.
3861a8c to
c9a257d
Compare
Thanks @ileodo, I’ve updated #37 against main and adapted the detection to your SchemaProfile and SchemaProfileResolver from #42. |
This branch has not been deployed
2 participants
Summary
This PR adds compatibility profiles for three observed MoneyWiz SQLite investment-column layouts: unsuffixed, suffixed, and mixed consumer-specific.
Unknown or ambiguous layouts are rejected rather than mapped speculatively.
MoneyWiz has changed its physical database columns between versions. Investment values may use suffixed names such as
ZNUMBEROFSHARES1, unsuffixed names such asZNUMBEROFSHARES, or a mixture where holdings and transactions use different columns.Without schema-aware parsing, the API can fail to load a valid database or silently read a financial value belonging to another entity type.
Why the redesign was necessary
Selecting the first available column alias is unsafe because Core Data rows can expose both columns simultaneously:
ZNUMBEROFSHARESZNUMBEROFSHARES1Both values can be valid decimals, so selecting the wrong one may not produce an obvious error.
The implementation now detects the complete schema topology once and selects an explicit alias for each consumer.
Schema profiles
Motivation
Individual model rows do not contain enough context to decide what each physical column represents. The decision must be based on the database schema rather than whichever value appears first.
Implementation
src/moneywiz_api/schema_profile.pyinspectsZSYNCOBJECTusingPRAGMA table_infoand identifies:Ambiguous schemas are rejected instead of risking incorrect financial data.
src/moneywiz_api/database_accessor.pydetects and retains this profile when opening the database so every record receives the same interpretation.Investment parsing
Motivation
Investment holdings and investment transactions are separate schema consumers. They cannot safely share one global “number of shares” alias.
Buy and sell transactions must also follow the same policy; fixing only one would leave inconsistent transaction behavior.
Implementation
src/moneywiz_api/model/investment_holding.pyuses the holding-specific share column.src/moneywiz_api/model/transaction.pyuses transaction-specific share and price columns for both buy and sell transactions.src/moneywiz_api/model/raw_data_handler.pycentralizes required, nullable, aliased, and profile-selected decimal parsing.Transfer compatibility
Motivation
Some MoneyWiz transfer rows omit a derived amount even when the canonical transaction amount and exchange metadata remain available.
Reconstructing the value blindly is unsafe because a zero exchange rate is invalid and a rounded exchange rate may not reproduce the exact value stored by MoneyWiz.
Implementation
src/moneywiz_api/model/transaction.pynow:Best-effort record loading
Motivation
One malformed or unsupported row should not prevent thousands of otherwise valid records from loading.
At the same time, skipped records must remain observable so callers can diagnose incomplete results.
Implementation
src/moneywiz_api/managers/record_manager.pyadds:load_errorscollection containing the record ID, entity type, and exception type.Unexpected programming and infrastructure errors continue to propagate.
Reduced-schema compatibility
Motivation
Older, reduced, and fixture databases may omit optional MoneyWiz relationship tables or filtered metadata columns.
Those omissions should produce empty optional relationships rather than prevent the main records from loading.
Implementation
src/moneywiz_api/database_accessor.pyverifies optional tables before querying them.src/moneywiz_api/model/raw_data_handler.pytolerates absent optional filtered fields.Regression coverage
tests/unit/test_schema_profiles.pycovers suffixed, unsuffixed, mixed, and ambiguous schemas.tests/unit/test_profiled_investment_values.pyverifies consumer-specific values for holdings and both buy and sell transactions.tests/unit/test_transaction_compatibility.pycovers transfer reconstruction, zero-rate rejection, rounding preservation, and continued loading after an invalid record.Validation
uv run pytest -q tests/unit— 23 passeduv run ruff check src tests/unit— passeduv run ruff format --check src tests/unit— passedgit diff --check— passed