Ujjwaljain16 commented on PR #43974: URL: https://github.com/apache/superset/pull/43974#issuecomment-5581394440
as per bito code review additional suggestion: that's literally the case this fix is meant to handle i went back and forth on this, but ended up leaving it exact on purpose the two ways this can go wrong aren't equally bad here if it stays too strict, worst case someone hits the same denial they'd get today annoying but not new if i make it case-insensitive and am wrong about some engine's collation i have just taught the authorization check to treat two different databases as the same one that's a much scarier failure to ship and "same database, different case" isn't really something python can decide on its own it depends on the engine's collation, which is configurable MSSQL happens to default to case-insensitive but that's a default, not a guarantee, so i didn't want to bake that assumption in even just for MSSQL. i did look at denormalize_name first, hoping it'd solve this for free but it's solving a different problem (round-tripping identifiers through SQLAlchemy's reflection layer), not "are these two names equal under this engine's collation." since we don't have anything in the codebase that actually knows that, i left the comparison exact and added a comment explaining the reasoning if it's worth fixing for MSSQL specifically, i think it deserves a real design pass around collation rather than a .casefold() slapped on here happy to take that on separately if it's wanted... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
