From e0b7797fe4e5a1c0e5f53df4f6cddb6e615da42b Mon Sep 17 00:00:00 2001
From: Amr ALHOSSARY <amr_alhossary@hotmail.com>
Date: Tue, 18 Aug 2026 10:22:02 -0400
Subject: [PATCH] Write the entry identifier as a data item, not only as the
 block name

AbstractCifFileSupplier put the PDB identifier only in the data_ block
header, so it was lost for every consumer that reads it as a data item:

  - BioJava's own CifStructureConsumerImpl.consumeStruct reads
    _struct.entry_id, so Structure.getPdbId() came back null after a
    write-then-read round trip;
  - Jmol's mmCIF reader takes _M.pdbID from _entry.id, so a structure
    handed over with openStringInline(structure.toMMCIF()) arrived with
    no identifier, which in turn breaks anything keyed on the entry
    (for instance "isosurface ... eds", which needs an entry to fetch
    the electron density map for).

Write both categories when the structure has an identifier. Both are
needed: _entry.id is the canonical item and the one Jmol reads, while
BioJava's own reader only looks at _struct.entry_id.

Adds a round-trip regression test; there was nothing asserting that an
identifier survived toMMCIF().

Fixes #1143
---
 .../io/cif/AbstractCifFileSupplier.java       | 21 +++++++++-
 .../io/cif/CifFileSupplierImplTest.java       | 40 +++++++++++++++++++
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/AbstractCifFileSupplier.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/AbstractCifFileSupplier.java
index 6bf8af90ef..e43565c827 100644
--- a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/AbstractCifFileSupplier.java
+++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/AbstractCifFileSupplier.java
@@ -40,8 +40,27 @@ protected CifFile getInternal(Structure structure, List<WrappedAtom> wrappedAtom
         // entity information
         List<EntityInfo> entityInfos = structure.getEntityInfos();
 
+        PdbId pdbId = structure.getPdbId();
+
         MmCifBlockBuilder blockBuilder = CifBuilder.enterFile(StandardSchemata.MMCIF)
-                .enterBlock(structure.getPdbId() == null? "" : structure.getPdbId().getId());
+                .enterBlock(pdbId == null? "" : pdbId.getId());
+
+        if (pdbId != null) {
+            // The block header alone does not carry the identifier for consumers: readers pick it up from
+            // _entry.id (e.g. Jmol) or from _struct.entry_id (BioJava's own CifStructureConsumerImpl).
+            // Both are written so that the identifier survives a write-then-read round trip either way.
+            blockBuilder.enterEntry()
+                    .enterId()
+                    .add(pdbId.getId())
+                    .leaveColumn()
+                    .leaveCategory();
+
+            blockBuilder.enterStruct()
+                    .enterEntryId()
+                    .add(pdbId.getId())
+                    .leaveColumn()
+                    .leaveCategory();
+        }
 
         blockBuilder.enterStructKeywords().enterText()
         .add(String.join(", ", structure.getPDBHeader().getKeywords()))
diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/io/cif/CifFileSupplierImplTest.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/io/cif/CifFileSupplierImplTest.java
index df227a8669..1d5f496ff5 100644
--- a/biojava-structure/src/test/java/org/biojava/nbio/structure/io/cif/CifFileSupplierImplTest.java
+++ b/biojava-structure/src/test/java/org/biojava/nbio/structure/io/cif/CifFileSupplierImplTest.java
@@ -1,5 +1,6 @@
 package org.biojava.nbio.structure.io.cif;
 
+import org.biojava.nbio.structure.PdbId;
 import org.biojava.nbio.structure.Structure;
 import org.biojava.nbio.structure.io.FileParsingParameters;
 import org.biojava.nbio.structure.io.PDBFileParser;
@@ -41,4 +42,43 @@ public void shouldReadRawPdbOutputtingCifWithEntity() throws IOException {
         }
 
     }
+
+    /**
+     * The identifier must be written as a data item and not only as the name of the data block: consumers read it
+     * from _entry.id or from _struct.entry_id, so writing the block header alone loses it. See issue #1143.
+     */
+    @Test
+    public void shouldWriteEntryIdAndSurviveRoundTrip() throws IOException {
+        Structure s;
+        try (InputStream inStream = new GZIPInputStream(this.getClass().getResourceAsStream("/4hhb.cif.gz"))) {
+            s = CifStructureConverter.fromInputStream(inStream);
+        }
+        assertEquals(new PdbId("4HHB"), s.getPdbId());
+
+        String cifText = CifStructureConverter.toText(s);
+        assertTrue("_entry.id must be written", cifText.contains("_entry.id"));
+        assertTrue("_struct.entry_id must be written", cifText.contains("_struct.entry_id"));
+
+        Structure readStruct = CifStructureConverter.fromInputStream(
+                new ByteArrayInputStream(cifText.getBytes()));
+
+        assertEquals(s.getPdbId(), readStruct.getPdbId());
+        assertEquals(s.getPdbId(), readStruct.getPDBHeader().getPdbId());
+    }
+
+    /**
+     * Structures without an identifier must not gain empty entry categories.
+     */
+    @Test
+    public void shouldNotWriteEntryIdWhenPdbIdIsAbsent() throws IOException {
+        Structure s;
+        try (InputStream inStream = new GZIPInputStream(this.getClass().getResourceAsStream("/4hhb.cif.gz"))) {
+            s = CifStructureConverter.fromInputStream(inStream);
+        }
+        s.setPdbId(null);
+
+        String cifText = CifStructureConverter.toText(s);
+        assertFalse(cifText.contains("_entry.id"));
+        assertFalse(cifText.contains("_struct.entry_id"));
+    }
 }