Repository navigation
fix: address the review findings before the first tag - #2
Merged
Merged
Conversation
The upsert caught only \Exception, so an Error (a TypeError raised deep in a save, for instance) escaped without a rollback and left the transaction open for the rest of the request. The transaction now runs through the category resource model's beginTransaction/commit/rollBack, the API Magento's own category code uses, instead of the raw adapter. Live check on 2.4.9: the raw adapter path already ran the after-commit callbacks, because core registers ExecuteCommitCallbacks as a plugin on AdapterInterface::commit and rollBack (also present in 2.4.6), so catalog_category_save_commit_after fired at transaction level 0 and the flat table was updated before and after this change alike. The switch keeps the module off that global plugin rather than fixing a lost reindex. composer test 39/39, phpcs clean, phpstan level 6 clean.
The upsert had two code paths that disagreed. Names containing a slash went through the category repository, which matched names case-sensitively, did not see an inactive sibling and so saved a second "Men" that failed on the url_key collision, and read names in the request's store. Every other path went through the native processor, which silently created an unknown root as a brand new tree. The repository path is gone. Every path now goes through the native CatalogImportExport processor, with a slash inside a name escaped as the processor's own "\/" quoting, and a name ending with a backslash rejected because the processor cannot express it. The root is checked up front at store 0, case-insensitively as the processor compares names, and an unknown root answers 400 before anything is written. The parser recognises the root case-insensitively for the same reason. Live on 2.4.9: missing root 400 with no row created, an inactive "Men" reused, "Men/Shirts" twice gives the same id, "T/Shirts" stored literally, "women|A/B" lands under "Women", a call through /rest/default writes store 0 only, no duplicate siblings.
Two simultaneous calls creating the same new path raced: live on 2.4.9, four parallel "Race/Alpha/Beta" calls left two answering 400 on the url rewrite collision. The native processor also loads the whole tree in its constructor, so an injected shared instance answered from the tree as it was when the request started. The upsert now holds Magento's LockManagerInterface lock dagster_bridge_category_upsert for the whole read and write, and builds a fresh processor only once it holds the lock, through a module-owned factory, so it sees what the previous holder committed. A call that cannot get the lock in 15 seconds answers 503 with a retry message. Live: six parallel calls for two new paths all answered 200 with the same ids and created exactly one set of contiguous ids; with the lock held from another DB session, the call answered 503 after 15 seconds and wrote nothing.
Two classes of attribute code produced wrong answers. A static code that is not a column of catalog_product_entity (category_ids, media_gallery) built an unknown column: live on 2.4.9 the call failed with the raw SQL error text in the response. A code whose values live elsewhere (tier_price) was read from prefix plus backend type and silently answered null for every product. The query builder now names the codes it cannot read: a static code must be a column of the product table (checked with describeTable), and any other code must resolve, through the attribute's own getBackendTable(), to one of the five standard value tables and have a scalar backend. The scalar check is what catches tier_price: it declares no backend table, so getBackendTable() names the decimal table, but its backend reports isScalar() false, the signal Magento's own EAV save uses. The service answers 400 listing every such code, like the existing unknown-code error, and the value select reads the table getBackendTable() resolved. Live: category_ids, tier_price and media_gallery each answer 400 naming them; sku, created_at, name, price, status and quantity_and_stock_status still read correctly.
The index and attribute value queries join on the metadata link field, and a test named ...OnCommerceStaging asserted that the join used row_id, which read as a claim of staging support. With content staging a product has one row per staged version, so both queries would answer one row per version: the test locked in behaviour that is wrong there. The module targets Open Source only. The test now pins just what is true, that the join column comes from the metadata pool, the code comments say staging is not supported and why, and README Requirements states that the endpoints are Open Source only. No behaviour change. Live on 2.4.9 Open Source: catalog_product_entity has no row_id column and products/index answers one row per product.
The SKU filter runs under MySQL's case-insensitive collation, so asking for "tst-conf-simp-s-gray" matched the row "TST-Conf-Simp-S-Gray". The rows were then stored under the database spelling while the items were built from the requested spelling, so the caller got null for both values: live on 2.4.9, name and sku both came back empty. Values are now keyed by the lowercased SKU and every item answers under the spelling the caller sent. Live: a lowercase and an uppercase spelling of two stored SKUs both answer their name, and the sku code returns the stored spelling.
Three inputs slipped past validation. An empty skus list with codes reached the query builder, which renders IN (). An empty category separator reached explode(), whose ValueError escaped as an uncaught Error: live on 2.4.9 the call answered HTTP 500 with a PHP stack trace as the body. A store_id naming no store was accepted and quietly answered default values only, which looks like "no store override". The attribute values call now answers 400 for an empty skus or attribute_codes list and for a store the store repository does not know; the category upsert answers 400 for an empty separator, before anything else runs. The library never sends empty lists (it chunks and skips), so the stricter contract does not affect it. Live: each of the four inputs answers 400 with its own message; store 0 and store 2 still answer.
composer.json hard-coded "version": "1.0.0". Composer skips a VCS tag whose version disagrees with that field, so the first tag cut without bumping it would silently not install. The capabilities endpoint read its version from that same field, which is why it existed. The version now lives in Capabilities::VERSION, with a unit test that fails when it differs from the top release heading of CHANGELOG.md and a one-line release checklist in the changelog. composer.json carries no version, and a test pins that. Every magento/* requirement was "*". They now follow the components Magento 2.4.6 and 2.4.9 ship, read from the Mage-OS mirror metadata (framework ~103.0.6, catalog ~104.0.6, catalog-import-export ~101.1.6, store ~101.1.6, backend ~102.0.6), the PHP range is explicit, and magento/module-eav is required and sequenced because the code uses its classes and never declared it. The tilde ranges admit later 2.4 patch lines, as Magento keeps them compatible. The Mage-OS repositories block stays: Packagist hosts none of these packages, and Composer reads repositories from the root package only, so it serves development and is ignored by stores; README says so. A new CI job installs with --prefer-lowest on PHP 8.1. Verified: composer validate --strict; a --prefer-lowest install under PHP 8.1.34 resolved framework 103.0.6, catalog 104.0.6, eav 102.1.6 and passed phpcs, phpstan and 56 unit tests; live capabilities still answers 1.0.0.
README claimed PHPStan runs with bitexpert/phpstan-magento, which is neither required in composer.json nor configured in phpstan.neon.dist; the claim is removed rather than adding an extension the analysis does not need. The tagged install used ^1.0, which cannot resolve before a tag exists, and is now marked as the instruction for once 1.0.0 is tagged. The branch install pointed at the merged feat/bridge-v1 branch and told the store to lower minimum-stability; it now requires dev-master, which a dry-run composer require against the 2.4.9 sandbox resolved (dev-master 1e374c6) with minimum-stability left at stable. The integration paragraph that repeated Access control is gone. CHANGELOG keeps [1.0.0] unreleased and lists every review fix.
The SQL builder tests assert substrings against a quoteInto double that pastes values in unescaped, so they prove the shape of the query and nothing about injection safety. A new test binds a hostile SKU and one with an apostrophe through a double that escapes like MySQL string literals and asserts both stay inside their literals; with the double reverted to raw pasting the test fails, so it does test the binding path. README Development states what injection safety actually rests on: quoteInto placeholders for every caller value and module-resolved identifiers only. Live on 2.4.9, the same two SKUs answer one empty item per pair and leak no rows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the ten confirmed review findings on the bridge before the first tag. One commit per finding, each with a failing test first where the behaviour is unit-testable, and each verified against a live Magento 2.4.9 Open Source store (developer mode, admin token,
/rest/all/unless stated) running this branch fromapp/code.CHANGELOG.mdkeeps[1.0.0] - unreleasedand gains a Fixed section. Nothing is tagged or released.One finding did not reproduce as stated (1). The raw adapter transaction did not lose the after-commit callbacks: core registers
Magento\Framework\Model\ExecuteCommitCallbacksas a plugin onAdapterInterface::commit/rollBack(inapp/etc/di.xmlof 2.4.9, and of 2.4.6 upstream), socatalog_category_save_commit_afterfired at transaction level 0 and category flat rows were written both before and after the change. The commit keeps the decided switch to the category resource model's transaction API, and the part that was really broken, rolling back on\Throwableand not only\Exception.Commits
fix(categories): roll back on any Throwable via the category resourcetestAnErrorThatIsNotAnExceptionStillRollsBack,testSuccessCommitsThroughTheCategoryResourceModelfix(categories): always use the native processor and validate the roottestASlashInsideANameIsEscapedForTheNativeProcessor,testMissingRootIsRejectedBeforeAnythingIsCreated,testRootMatchesCaseInsensitivelyAsTheNativeProcessorDoes,testTheRootIsReadAtTheAdminStore,testANameEndingWithABackslashIsRejected, parsertestParseRecognisesTheRootCaseInsensitively...fix(categories): serialise concurrent upserts with a named locktestTheProcessorIsBuiltUnderTheLockAndTheLockIsReleased,testTheLockIsReleasedWhenTheUpsertFails,testTheLockIsNamedAndBounded,testALockTimeoutAnswersServiceUnavailablefix(attribute-values): refuse attributes outside the standard tablestestStaticAttributeThatIsNotAProductColumnIsUnsupported,testAttributeWhoseBackendIsNotScalarIsUnsupported,testAttributeWithACustomBackendTableIsUnsupported,testUnsupportedAttributeCodesAreRejectedAndListedfix(index): declare Commerce content staging unsupportedtestStatusJoinFollowsTheLinkFieldOnCommerceStagingreplaced bytestStatusJoinTakesTheLinkFieldFromTheMetadataPool(no behaviour change)fix(attribute-values): match SKUs case-insensitively like the databasetestASkuSpelledInAnotherCaseAnswersUnderTheRequestedSpelling,testAStaticValueIsFoundForASkuSpelledInAnotherCasefix(validation): reject empty lists, empty separators and unknown storestestEmptySkusAreRejected,testEmptyAttributeCodesAreRejected,testAStoreThatDoesNotExistIsRejected,testAnEmptySeparatorIsRejectedfix(composer): drop the version field and pin real Magento rangestestVersionIsTheTopReleaseHeadingOfTheChangelog,testComposerJsonCarriesNoVersionFieldfix(docs): make the install and development notes truefix(test): pin value binding through an escaping quote doubletestCallerValuesAreBoundThroughTheAdapterQuoting(a coverage test: passes on the existing code, and fails when the double is mutated to paste raw values)Design notes
Model/CategoryFactory.phpare gone. Every path goes through the nativeCategoryProcessor, a/inside a name is escaped as the processor's own\/, and a name ending with\is rejected (the processor cannot express it). The root is checked at store 0, case-insensitively as the processor compares names. The native semantics are documented in README ("Category upsert semantics").CategoryProcessorFactory. Lock timeout 15 s, then aWebapi\Exceptionwith HTTP 503.tier_pricedeclares nobackend_table, sogetBackendTable()resolves tocatalog_product_entity_decimaland a table check alone does not catch it. Its backend reportsisScalar() === false, the signal Magento's own EAV save uses, so the check is: static codes must be product table columns (describeTable), other codes must resolve throughgetBackendTable()to one of the five standard tables and have a scalar backend.magento/product-community-edition2.4.6 and 2.4.9 (framework 103.0.6 / 103.0.9, catalog 104.0.6 / .9, catalog-import-export 101.1.6 / .9, store 101.1.6 / .9, backend 102.0.6 / .9, eav 102.1.6 / .9).magento/module-eavwas used but never required; it is now required and sequenced. The tilde ranges (~103.0.6, ...) also admit later 2.4 patch lines; say so if you prefer a hard cap at 2.4.9. The Mage-OSrepositoriesblock stays because Packagist hosts none of themagento/*packages (checked: 404 for all six) and Composer readsrepositoriesfrom the root package only; README Development explains it.Live evidence
POST categories/upsert {"paths":["FlatBefore/Child"]}thenFlatAfter/Child, category flat enabled, flat indexer Update on Save, a temporary observer loggingcatalog_category_save_after/_save_commit_afterwith the transaction levelsave_afterat txlevel 3,save_commit_afterat txlevel 0 for both idssave_commit_afterat txlevel 0catalog_category_flat_store_1in both runs: the callbacks were not lost before the change{"paths":["X"],"root":"No Such Root"}The root category "..." does not exist.Men(is_active=0), then{"paths":["Men|T/Shirts"],"separator":"|"}Men); repeated call answers 36 againT/Shirtsrow, stored literally{"paths":["women|A/B"],"separator":"|"}Women(id 6)womensibling{"paths":["Men/Shirts","Men/Shirts"]}then againPOST /rest/default/.../upsert {"paths":["Scope/Check"]}GET_LOCK('<db>|dagster_bridge_category_upsert')plusSLEEP)Another category upsert is still running after 15 seconds; retry the call later.attribute-values {"attribute_codes":["category_ids"]}Unknown column 'e.category_ids'), developer modecategory_ids{"attribute_codes":["tier_price","price"]}tier_pricecategory_ids, tier_price, media_gallery, namesku, created_at, name, price, status, quantity_and_stock_statusproducts/index?limit=2catalog_product_entityhas norow_idcolumn (Open Source){"skus":["tst-conf-simp-s-gray"],...}for a storedTST-Conf-Simp-S-Grayskureturns the stored spellingskus[]At least one SKU is required.attribute_codesAt least one attribute code is required.separator: ""ValueErrorstack trace as the bodyThe separator must not be empty.store_id: 999Store 999 does not exist.; store 0 and 2 still answerGET capabilities1.0.01.0.0fromCapabilities::VERSIONcomposer require ddtcorex/module-dagster-bridge:dev-masterin a copy of a 2.4.9 project withminimum-stability: stabledev-master 1e374c6{"skus":["x') OR 1=1 -- ","o'brien"],...}The temporary observer module, the flat config change and every test category were removed from the store afterwards.
Test plan
composer test: 57 tests green (PHPUnit 12.5, PHP 8.5)composer phpcs: cleancomposer phpstan: level 6, no errorscomposer validate --strictcomposer update --prefer-lowest --prefer-stableunder PHP 8.1.34 resolved framework 103.0.6, catalog 104.0.6, eav 102.1.6, PHPUnit 10.5, PHPStan 1.12.0 and passed phpcs, phpstan and all unit tests (run locally in a container; the new CI joblowestdoes the same)php 8.1, lowest dependenciesjobNot verified
app/etc/di.xmlcheck forExecuteCommitCallbacks.\Throwablethat is not anExceptionwas only exercised in unit tests; no live path produced one.dev-masterresolves today to the pre-fix commit; the fixed code reachesdev-masteronly after this PR merges.