Fix the remaining culture-sensitive lookups (Esent type names, unit symbols, property sets) - #673
Open
oguzozshn wants to merge 3 commits into
Open
Conversation
EntityCollection.OfType(string) already upper-cases with the invariant culture before hitting the metadata dictionary, which is keyed on invariant upper case. The Esent implementation used the culture-sensitive ToUpper(), so the same public API returned results or nothing depending on which store backed the model: under tr-TR "IfcBuilding" upper-cases to "IFCBUİLDİNG" and never matches. The added test uses IfcBuilding rather than IfcWall on purpose - a type name only trips the Turkish casing rules if it contains a lower case 'i'. Refs xBimTeam#670
IfcConversionBasedUnit.Symbol matched the unit name against "FEET", "FOOT" and "INCH" by upper-casing it first. ToUpper() is culture sensitive, so under the Turkish rules "inch" becomes "İNCH", which does not contain "INCH", and the symbol fell through to the raw name instead of "in". Replaced with an ordinal case-insensitive IndexOf, which also drops the intermediate allocation. Applied to all three schemas. Refs xBimTeam#670
GetPropertySet and GetElementQuantity implemented their caseSensitive: false option with culture-sensitive comparisons - string.Compare(a, b, ignoreCase), CurrentCultureIgnoreCase, and a pair of ToLower() calls. Turkish treats 'i' and 'I' as different letters, so under tr-TR looking up "Pset_WindowCommon" as "PSET_WINDOWCOMMON" found nothing. Property set names are IFC identifiers rather than prose, so the case-insensitive path now uses OrdinalIgnoreCase, and the case-sensitive path Ordinal. Ifc4x3 has no equivalent helpers. Refs xBimTeam#670
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.
Follow-up to #672, taking you up on the offer to look at the remaining culture-sensitive code.
Three separate defects, one commit each, each with a test that fails without its fix.
1. Express type name lookup in the Esent store
EntityCollection.OfType(string)already upper-cases with the invariant culture before hitting themetadata dictionary, which is keyed on invariant upper case.
PersistedEntityInstanceCache.OfTypeused
ToUpper(), so the same public API returned results or nothing depending on which store backedthe model. Under
tr-TR,"IfcBuilding"upper-cases to"IFCBUİLDİNG"and never matches.The test deliberately queries
IfcBuildingrather thanIfcWall— a type name only trips theTurkish casing rules if it contains a lower case
i, andIfcWalldoes not.2. Imperial unit symbols
IfcConversionBasedUnit.Symbolmatched the unit name against"FEET","FOOT"and"INCH"afterupper-casing it. Under
tr-TR,"inch"becomes"İNCH", which does not contain"INCH", so thesymbol fell through to the raw name instead of
"in". Replaced with an ordinal case-insensitiveIndexOf, which also drops the intermediate allocation. All three schemas.3. Property set lookups
GetPropertySetandGetElementQuantityimplemented theircaseSensitive: falseoption withculture-sensitive comparisons —
string.Compare(a, b, ignoreCase), one explicitCurrentCultureIgnoreCase, and a pair ofToLower()calls. Property set names are IFC identifiersrather than prose, so these now use
OrdinalIgnoreCase, and the case-sensitive pathOrdinal.Ifc4x3has no equivalent helpers.Deliberately left alone
I went through every remaining
ToUpper()/ToLower()in the tree and did not touch:GetType().Name.ToUpper()insideXbimParserExceptionmessages. They only affect the text of anerror, and they come from the code generator rather than these files.
XbimXmlReader3'sswitch (pt.ToString().ToLower())— its only labels are"string"and"boolean"; neither contains a lower casei, so the casing cannot change the outcome.IPersistEntityExtensions— I had changed this, then reverted it afterconfirming every IFC enum member is already upper case ASCII, which makes
ToUpper()a no-opthere. It seemed better to leave the diff honest than to pad it.
Happy to fold any of these in if you would rather have the codebase uniform.
Verification
On a
tr-TRmachine:Xbim.Essentials.Tests500/500 on bothnet10.0andnet48,Xbim.Essentials.NetCore.Tests94/94. Each fix was also checked by stashing it and confirming itstest fails.
Refs #670.