-
-
Notifications
You must be signed in to change notification settings - Fork 96
fix(query)!: reject a non-IFC type name in ofType(), but not standard types the enum table omits #3009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
fix(query)!: reject a non-IFC type name in ofType(), but not standard types the enum table omits #3009
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
33bda64
fix(query): reject an unrecognized IFC type name in ofType() instead …
BIMvoice 8df2d29
fix(query): let standard-but-unmapped IFC types through ofType()'s guard
BIMvoice 86b1d13
Merge branch 'main' into query-diff-create-sweep
louistrue 30e1926
fix(query): widen ofType()'s oracle to every schema the parser reads
BIMvoice a52497a
fix(parser,codegen): stop the schema registry answering for Object.pr…
BIMvoice 02b25ef
fix(query): trim ofType()'s name once, so the guard and the resolutio…
BIMvoice 999bc6c
revert(codegen,parser): hand the prototype-chain fix back to #3069
BIMvoice 912473d
Merge branch 'main' into query-diff-create-sweep
louistrue bd9fab6
chore(changeset): drop the #3069 ordering note, which ships to the CH…
louistrue File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| --- | ||
| "@ifc-lite/query": major | ||
| --- | ||
|
|
||
| **Breaking:** `IfcQuery.ofType()` now throws for a type string that is not an IFC entity name, instead of silently querying the `Unknown` bucket. | ||
|
|
||
| `ofType()` maps each type string through `IfcTypeEnumFromString`, which falls back to `IfcTypeEnum.Unknown` for any name it does not recognize. A typo — `ofType('IfcWal')` — therefore returned every entity whose type the store could not classify: neither the caller's walls nor an empty result, but some other, unrelated set of entities. `ofType()` now rejects such a string with an error naming it. | ||
|
|
||
| What still works unchanged: | ||
|
|
||
| - **Standard IFC types that this build's enum table does not map.** `TYPE_STRING_TO_ENUM` (`@ifc-lite/data`) is a curated subset of IFC, so standard buildingSMART types such as `IfcChiller`, `IfcActuator`, `IfcElectricAppliance` — and IFC2X3's `IfcDoorStyle`, `IfcWindowStyle` and `IfcElectricalDistributionPoint` — resolve to `Unknown`. These are **not** rejected: they keep falling through to the `Unknown` bucket exactly as before, which is the only representation this build has for them and which answers the query correctly in a file whose unclassified entities are of that type. | ||
|
|
||
| The oracle deciding this is `isKnownType()` (`@ifc-lite/parser`), the predicate that already guards `@ifc-lite/sdk`'s `addEntity`: the bundled **IFC2X3 + IFC4 + IFC4X3** schema union, minus EXPRESS defined types (`IfcLengthMeasure`, `IfcArcIndex`), with the IFC4_ADD2_TC1 codegen pin as a fallback, plus the parser's alias table for IFC2X3 leaves the bundled EXPRESS exports omit. Reusing it rather than adding a second name table keeps one source of truth for "is this a real IFC class". The suite asserts the coverage exhaustively — every entity in `SCHEMA_REGISTRY` and in all three per-version tables must pass `ofType()` — rather than by sampling names. | ||
| - **The `Unknown` bucket itself**, still reachable by passing the literal string `'Unknown'`. | ||
|
|
||
| What breaks: a call passing a name that is not an IFC entity name in any of those schemas — a typo, or a genuine vendor-specific type name — previously returned an `EntityQuery` over the `Unknown` bucket and now throws. Callers relying on a vendor-specific name to reach unclassified entities must pass `'Unknown'` instead. Hence the major bump: this is a behaviour change on a published SDK export, not a bug fix that is invisible to correct callers. | ||
|
|
||
| The error text says which schemas were searched rather than assuming a misspelling, because a rejected name may well be spelled correctly: | ||
|
|
||
| > `ofType(): "IfcWal" is not an entity name in any IFC schema this build reads (IFC2X3, IFC4, IFC4X3). Check the spelling; for a vendor-specific type name, pass 'Unknown' to query entities whose type could not be classified.` |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,239 @@ | ||
| /* This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /** | ||
| * `IfcQuery.ofType()` maps a type string through `IfcTypeEnumFromString`, | ||
| * which falls back to `IfcTypeEnum.Unknown` for any name it does not | ||
| * recognize. That single fallback covers two different situations, and only | ||
| * one of them is a caller error: | ||
| * | ||
| * - `'IfcWal'` is not an IFC entity name at all, so the caller meant | ||
| * `'IfcWall'`. Silently answering with the Unknown bucket - every entity | ||
| * the store could not classify - returns some other, unrelated set of | ||
| * entities. `ofType()` rejects this. | ||
| * | ||
| * - `'IfcChiller'` IS a standard IFC4 entity name; `TYPE_STRING_TO_ENUM` | ||
| * (packages/data/src/types.ts) is a curated subset that has no row for it, | ||
| * so it maps to Unknown as well. The Unknown bucket is the only | ||
| * representation this build has for such an entity, and querying it is the | ||
| * correct, pre-existing behaviour. `ofType()` must NOT reject these. | ||
| * | ||
| * The discriminator therefore has to be an oracle that spans every schema the | ||
| * parser reads, not one of them. An earlier revision keyed the check on | ||
| * `IFC_ENTITY_NAMES` - IFC4X3-only, and hand-maintained - which rejected | ||
| * `IfcDoorStyle` and `IfcWindowStyle`, the entities IFC2X3 files use to carry | ||
| * door and window typing. The exhaustive sweeps below exist so that a | ||
| * schema-coverage hole cannot pass again: a hand-picked sample of five names | ||
| * that all happen to sit in one table cannot see it. | ||
| * | ||
| * See `ifc-query.ts`. | ||
| */ | ||
|
|
||
| import { describe, it, expect } from 'vitest'; | ||
| import { | ||
| ENTITIES_IFC2X3, | ||
| ENTITIES_IFC4, | ||
| ENTITIES_IFC4X3, | ||
| IFC_DATA_TYPES, | ||
| } from '@ifc-lite/data'; | ||
| import { SCHEMA_REGISTRY, isKnownType } from '@ifc-lite/parser'; | ||
| import { createMockStore } from './mock-store.js'; | ||
| import { IfcQuery } from '../src/ifc-query.js'; | ||
|
|
||
| /** | ||
| * Standard buildingSMART entity names that `TYPE_STRING_TO_ENUM` has no entry | ||
| * for. Each maps to `IfcTypeEnum.Unknown`, so a rule keyed on "did this map to | ||
| * Unknown?" alone would wrongly reject every one of them. The last two are the | ||
| * IFC2X3 door/window typing entities that the `IFC_ENTITY_NAMES` oracle | ||
| * rejected. | ||
| */ | ||
| const STANDARD_BUT_UNMAPPED = [ | ||
| 'IfcChiller', | ||
| 'IfcActuator', | ||
| 'IfcElectricAppliance', | ||
| 'IfcBuildingSystem', | ||
| 'IfcAudioVisualAppliance', | ||
| 'IfcDoorStyle', | ||
| 'IfcWindowStyle', | ||
| // IFC2X3 leaf that no bundled EXPRESS export carries; the parser's | ||
| // `ENTITY_NAME_ALIASES` is the only table that knows it. | ||
| 'IfcElectricalDistributionPoint', | ||
| ] as const; | ||
|
|
||
| /** | ||
| * Names that are not IFC entity names in ANY schema this build reads, so | ||
| * `ofType()` must keep rejecting them. Without this direction a guard that | ||
| * accepted everything would pass the exhaustive sweeps below while having | ||
| * removed the feature entirely. | ||
| */ | ||
| const NOT_IFC_ENTITY_NAMES = [ | ||
| 'IfcWal', // the typo this guard exists for | ||
| 'IfcWalll', | ||
| 'IFCPROPRIETARYVENDORTHING', | ||
| 'Wall', | ||
| 'IfcLengthMeasure', // a real IFC *defined type*, not an entity | ||
| '', | ||
| ] as const; | ||
|
|
||
| function storeWithUnclassified(unclassifiedType: string) { | ||
| return createMockStore({ | ||
| entities: [ | ||
| { expressId: 10, type: 'IFCWALL', globalId: 'g10', name: 'Real Wall' }, | ||
| { | ||
| expressId: 20, | ||
| type: unclassifiedType.trim().toUpperCase(), | ||
| globalId: 'g20', | ||
| name: 'Unclassified', | ||
| }, | ||
| ], | ||
| }); | ||
| } | ||
|
|
||
| describe('ofType() rejects a type string that is not an IFC entity name', () => { | ||
| it('throws on a typo rather than silently matching the Unknown bucket', () => { | ||
| const query = new IfcQuery(storeWithUnclassified('IFCCHILLER') as any); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| // Caller made a typo: 'IfcWal' instead of 'IfcWall'. | ||
| expect(() => query.ofType('IfcWal')).toThrow(/is not an entity name in any IFC schema/); | ||
| }); | ||
|
|
||
| it('throws on a name that is not in the IFC schema at all', () => { | ||
| const query = new IfcQuery(storeWithUnclassified('IFCCHILLER') as any); | ||
| expect(() => query.ofType('IFCPROPRIETARYVENDORTHING')).toThrow( | ||
| /is not an entity name in any IFC schema/, | ||
| ); | ||
| }); | ||
|
|
||
| it('rejects a bad name even when a good one is passed alongside it', () => { | ||
| const query = new IfcQuery(storeWithUnclassified('IFCCHILLER') as any); | ||
| expect(() => query.ofType('IfcWall', 'IfcWal')).toThrow(/is not an entity name in any IFC schema/); | ||
| }); | ||
|
|
||
| it('still allows an explicit query for the Unknown bucket itself', async () => { | ||
| const query = new IfcQuery(storeWithUnclassified('IFCPROPRIETARYVENDORTHING') as any); | ||
| const ids = await query.ofType('Unknown').ids(); | ||
| expect(ids).toEqual([20]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('ofType() accepts standard IFC types the enum table does not map', () => { | ||
| for (const typeName of STANDARD_BUT_UNMAPPED) { | ||
| it(`${typeName} does not throw and still reaches the Unknown bucket`, async () => { | ||
| const query = new IfcQuery(storeWithUnclassified(typeName) as any); | ||
| expect(() => query.ofType(typeName)).not.toThrow(); | ||
| // The store's only unclassified entity is the one of this very type, so | ||
| // the Unknown bucket answers the query correctly - as it did before the | ||
| // guard existed. Entity 10 (a mapped IfcWall) must not leak in. | ||
| const ids = await query.ofType(typeName).ids(); | ||
| expect(ids).toEqual([20]); | ||
| }); | ||
| } | ||
|
|
||
| it('accepts a standard unmapped type in any casing, with surrounding space', () => { | ||
| const query = new IfcQuery(storeWithUnclassified('IfcChiller') as any); | ||
| expect(() => query.ofType('IFCCHILLER')).not.toThrow(); | ||
| expect(() => query.ofType(' ifcchiller ')).not.toThrow(); | ||
| }); | ||
| }); | ||
|
|
||
| /** | ||
| * The exhaustive sweeps. Every entity name in a schema table this build ships | ||
| * must survive `ofType()`; a single rejection is a name a real file can carry | ||
| * and a correctly spelled query cannot reach. | ||
| */ | ||
| describe('ofType() accepts every entity name in every schema this build reads', () => { | ||
| const query = () => new IfcQuery(storeWithUnclassified('IFCCHILLER') as any); | ||
|
|
||
| /** | ||
| * The upstream SchemaInfo tables carry EXPRESS *defined types* | ||
| * (`IfcLengthMeasure`, `IfcBoolean`, `IfcArcIndex`, ...) as rows alongside | ||
| * real ENTITY declarations, because IDS needs their names. They are not | ||
| * entity names, so they are not part of what `ofType()` promises to accept - | ||
| * subtract them rather than weakening the assertion to cover them. | ||
| * | ||
| * Two tables are needed to name them all, which is why this mirrors the | ||
| * parser's own subtraction (`NON_ENTITY_NAMES_UPPER` in | ||
| * `packages/parser/src/ifc-schema.ts`) instead of using `IFC_DATA_TYPES` | ||
| * alone: the IDS table omits six that `SCHEMA_REGISTRY.types` carries | ||
| * (`IfcBinary`, `IfcArcIndex`, `IfcLineIndex`, `IfcComplexNumber`, | ||
| * `IfcCompoundPlaneAngleMeasure`, `IfcPropertySetDefinitionSet`). | ||
| */ | ||
| const DEFINED_TYPES = new Set([ | ||
| ...IFC_DATA_TYPES.map((t) => t.name.toUpperCase()), | ||
| ...Object.keys(SCHEMA_REGISTRY.types).map((n) => n.toUpperCase()), | ||
| ...Object.keys(SCHEMA_REGISTRY.enums).map((n) => n.toUpperCase()), | ||
| ...Object.keys(SCHEMA_REGISTRY.selects).map((n) => n.toUpperCase()), | ||
| ]); | ||
|
|
||
| const entityNames = (table: readonly { name: string }[]) => | ||
| table.map((e) => e.name).filter((n) => !DEFINED_TYPES.has(n.toUpperCase())); | ||
|
|
||
| // The name the maintainer asked for by file: the parser's own registry, | ||
| // `packages/parser/src/generated/schema-registry.ts`. | ||
| it('accepts every entity in the parser SCHEMA_REGISTRY', () => { | ||
| const q = query(); | ||
| const names = Object.keys(SCHEMA_REGISTRY.entities); | ||
| expect(names.length).toBeGreaterThan(700); | ||
| const rejected = names.filter((n) => { | ||
| try { | ||
| q.ofType(n); | ||
| return false; | ||
| } catch { | ||
| return true; | ||
| } | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| expect(rejected).toEqual([]); | ||
| }); | ||
|
|
||
| for (const [schema, table] of [ | ||
| ['IFC2X3', ENTITIES_IFC2X3], | ||
| ['IFC4', ENTITIES_IFC4], | ||
| ['IFC4X3', ENTITIES_IFC4X3], | ||
| ] as const) { | ||
| it(`accepts every ${schema} entity name`, () => { | ||
| const q = query(); | ||
| const names = entityNames(table); | ||
| expect(names.length).toBeGreaterThan(400); | ||
| const rejected = names.filter((n) => { | ||
| try { | ||
| q.ofType(n); | ||
| return false; | ||
| } catch { | ||
| return true; | ||
| } | ||
| }); | ||
| expect(rejected).toEqual([]); | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| describe('ofType() still rejects names that are not IFC entity names', () => { | ||
| it('the rejected names really are unknown to the parser, not just to ofType()', () => { | ||
| // Pins the two directions to the SAME oracle: if a future change made | ||
| // `isKnownType` accept these, the sweeps above would still pass while the | ||
| // guard had quietly become a no-op. This fails first in that case. | ||
| for (const bad of NOT_IFC_ENTITY_NAMES) { | ||
| expect(isKnownType(bad)).toBe(false); | ||
| } | ||
| }); | ||
|
|
||
| for (const bad of NOT_IFC_ENTITY_NAMES) { | ||
| it(`rejects ${JSON.stringify(bad)}`, () => { | ||
| const q = new IfcQuery(storeWithUnclassified('IFCCHILLER') as any); | ||
| expect(() => q.ofType(bad)).toThrow(/is not an entity name in any IFC schema/); | ||
| }); | ||
| } | ||
|
|
||
| it('names the offending string and points at Unknown, without blaming spelling alone', () => { | ||
| const q = new IfcQuery(storeWithUnclassified('IFCCHILLER') as any); | ||
| let message = ''; | ||
| try { | ||
| q.ofType('IfcWal'); | ||
| } catch (e) { | ||
| message = (e as Error).message; | ||
| } | ||
| expect(message).toContain('"IfcWal"'); | ||
| expect(message).toContain('IFC2X3, IFC4, IFC4X3'); | ||
| expect(message).toContain("pass 'Unknown'"); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.