Skip to content

#3666, #3671, #3672, #3695 – Reject an SDF monomer library item with an invalid property - #4030

Open
MysterionRise wants to merge 1 commit into
masterfrom
3666-monomer-library-validation
Open

MysterionRise wants to merge 1 commit into
masterfrom
3666-monomer-library-validation

Conversation

@MysterionRise

Copy link
Copy Markdown
Collaborator

Generic request

  • PR name follows the pattern #1234 – issue name
  • branch name does not contain '#'
  • base branch (master or release/xx) is correct
  • PR is linked with the issue
  • task status changed to "In Review" — left to maintainers (not settable from a contributor account)
  • code follows product standards
  • regression tests updated

Optional

  • documentation updated — .memory-bank/features/monomer-library-sdf.md

Closes #3666
Closes #3671
Closes #3672
Closes #3695

What changed

Four issues from the #3161 checklist, all in the SDF reader of the monomer library (MonomerTemplateLibrary::addMonomersFromMolecule), which accepted items the requirement says must be rejected:

Issue Input Before Now
#3695 type unknown or empty took the preset path and loaded the templates as plain monomers Type: "wrongTypeHere" is unknown. "_Base1" monomer hasn't been added to the library.
#3666 preset without groupName (or groupClass), or with an empty one the guard contains("groupClass") && contains("groupName") skipped the preset silently and kept its templates Load of preset has failed, preset definition contains invalid groupName value. The groupName couldn't be empty (and the same for groupClass, naming the preset)
#3671 empty modificationTypes taken as no modification types Load of "_Peptide1" monomer has failed, monomer definition contains invalid modificationTypes value. The modificationTypes couldn't be empty
#3672 idtAliases entry without a value (base=, or ep3 with no =) dropped: split() drops empty tokens, so base= comes back as the single token base and failed the size() == 2 test Load of "_Nucleotide1" monomer has failed, monomer definition contains invalid idtAliases value. The value of "base" couldn't be empty

The messages for #3695 and #3671 are the ones the issues give; the other two follow their wording. All checks run before anything is added to the library and outside the try/catch that deliberately swallows duplicate templates. As with the existing "has disconnected templates" check, a failed check fails the whole load; Ketcher sends the whole SDF in one convert call and shows the message as Monomer item could not be loaded because of an error: <message>.

The four issues are in one PR because they edit the same function and the same test; separate PRs would conflict with each other.

Not in this PR

Compatibility. Every SDF in the repository (234 files) was scanned for a record the new checks reject: none. Ketcher's default library (api/wasm/indigo-ketcher/test/monomer_library.ket, 855 records, 21 presets) saved as SDF loads back with the checks: the saver writes only the two known types, always writes both group fields, and writes idtAliases and modificationTypes only when they have a value.

Tests

New api/tests/integration/tests/formats/monomer_library_properties.py, with inputs in tests/formats/molecules/monomer_library/ and references in tests/formats/ref/monomer_library/:

  • negative (error text in monomer_library_properties.py.out): the four issue inputs as reported, plus an empty groupName, a missing groupClass, an empty type, one empty entry among valid ones (ep5=5Nuc1;ep3=;internal=iNuc1), and an entry with no =.
  • positive (compare_diff against KET): a valid counterpart of each — the No error if preset loaded to the library without <groupName> defined #3666 preset with a groupName, modificationTypes Mod Type 1;Mod Type 2;, idtAliases ep5=…,ep3=…,internal=…, and an explicit type monomerTemplate.
Suites     integration harness, all files, CPython · core / C API / C++ wrapper GTest · Python wrapper unittest 15/15
Platforms  macOS arm64 (AppleClang 21, Ninja, Release)
Harness    the new test fails on master (all 9 invalid inputs load with no error) and passes with the fix;
           compared with master built the same way, no test fails with the fix that passes on master.
           13 tests fail on this Mac on master too (layout 7, formats 5, fingerprints 1) — platform.
Style      clang-format 11.0.1, isort, black, pflake8 in epmlsop/indigo-tester: clean; test compiles under Jython 2.7.2
Not run    Windows and Linux, Jython and IronPython runs — left to CI
Rebase     tested on d89316714; rebased onto aee95744e, whose 15 new commits touch none of the files this change reads

The error texts are new, so a Ketcher autotest asserting on them will need them.

🤖 Generated with Claude Code

…n invalid property

The SDF reader of the monomer library accepted items that #3161 says must
be rejected:
- an unknown or empty `type` sent the item down the preset path, which
  loaded its templates as plain monomers (#3695);
- a preset without `groupName` or `groupClass` skipped the preset and kept
  its templates (#3666);
- an empty `modificationTypes` was taken as no modification types (#3671);
- an `idtAliases` entry without a value was dropped, because the split on
  '=' drops empty tokens and `base=` comes back as one token (#3672).

All four are now checked before anything is added to the library, and the
error names the item, in the wording the issues ask for. The whole load
fails, as the existing check for a disconnected preset does; Ketcher shows
the message as "Monomer item could not be loaded because of an error".

tests/formats/molecules/monomer_library/*.sdf  new  the four issue inputs,
    the edges of each check, and a valid counterpart of each
tests/formats/ref/monomer_library/*.ket        new  what the valid
    counterparts load to
ref/formats/monomer_library_properties.py.out  new  the error of each
    invalid input
.memory-bank/features/monomer-library-sdf.md   new  the properties and
    their failure contract

verified: macOS arm64 (full integration harness, core/api/cpp GTest,
python wrapper tests), Jython compile, clang-format 11 / isort / black /
pflake8 in the CI tester image. Not run: Windows, Linux, IronPython.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant