Conversation
singhpk234
force-pushed
the
fix/prep-statement
branch
from
de4e7ad to
865fddf
Compare
singhpk234
force-pushed
the
fix/prep-statement
branch
from
845b698 to
09c32a3
Compare
singhpk234
requested review from
HonahX,
MonkeyCanCode,
RussellSpitzer,
adutra,
ajantha-bhat,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
pingtimeout,
snazy,
takidau and
vvcephei
as code owners
| private final RelationalJdbcConfiguration relationalJdbcConfiguration; | ||
| private final DatabaseType databaseType; | ||
| private final RelationalJdbcConfiguration relationalJdbcConfiguration; |
There was a problem hiding this comment.
nit: spurious change?
| QueryAction queryAction) | ||
| throws SQLException { | ||
| ModelEntity modelEntity = ModelEntity.fromEntity(entity); | ||
| ModelEntity modelEntity = ModelEntity.fromEntity(entity, realmId); |
There was a problem hiding this comment.
nit: Normally we pass in realmId first
|
|
||
| public static final String SCHEMA = "POLARIS_SCHEMA"; | ||
|
|
||
| public static final String ENTITY_LOOKUP_BY_CATALOG_ID_ID = |
There was a problem hiding this comment.
Can we name these a little different? I was confused by the ENTITY_ prefix. Maybe something like
QUERY_TEMPLATE_LOOKUP_BY_CATALOG_ID?
| + String.join(" = ? AND ", ModelPrincipalAuthenticationData.PK_COLUMNS) | ||
| + " = ?"; | ||
|
|
||
| public static final String PRINCIPAL_AUTHENTICATION_DATA_UPDATE_QUERY = |
There was a problem hiding this comment.
Sorry, the same comment on name applies to all these I guess...
| "securable_id", | ||
| "grantee_catalog_id", | ||
| "grantee_id", | ||
| "privilege_code"); |
There was a problem hiding this comment.
Can we add some check that PK_COLUMNS are a subset of ALL_COLUMNS?
There was a problem hiding this comment.
To here and other classes
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
After a quick glance this looks like a valuable improvement overall 👍
I hope we can still restructure the code a bit to better guard against possible SQL/param mismatches (specific comment below)
| List<Object> params = List.of(catalogId, parentId, typeCode, name, realmId); | ||
| return getPolarisBaseEntity( | ||
| QueryGenerator.generateSelectQuery( | ||
| ModelEntity.ALL_COLUMNS, ModelEntity.TABLE_NAME, params)); | ||
| new PreparedQuery(SQLConstants.ENTITY_LOOKUP_BY_NAME_QUERY, params)); |
There was a problem hiding this comment.
I'd prefer to keep this SQL constant and parameter bindings closer to each other to make it easier to maintain the SQL without breaking the binding.
For example: SQLConstants.entityLookupByName(catalogId, parentId, typeCode, name, realmId) -> PreparedQuery
WDYT?
There was a problem hiding this comment.
Well, that wouldn't be SQLConstants. Maybe we're back to QueryGenerator?
There was a problem hiding this comment.
Any class name would work for me. My point is that co-locating the SQL text (with placeholders) and the code that binds values to those placeholders makes maintenance easier.
|
Thank you so much for your feedbacks ! |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
3 participants
About the change
Static SQL is preferred over runtime SQL generation
Pending
Testing
Existing test pass