V3 casl 443 delete feature by id #342
Conversation
| class DeleteFeature(session: PgSession) : UpdateFeature(session) { | ||
| override fun execute(collection: PgCollection, write: Write): Tuple { | ||
| val featureId = write.featureId | ||
| require(featureId != null) {"No feature ID provided"} |
There was a problem hiding this comment.
Please always throw a NakshaException with NakshaError.ILLEGAL_ARGUMENT, in this case:
if (featureId == null) throw NakshaException(NakshaError.ILLEGAL_ARGUMENT, "No feature ID provided");
The reasoning is that we only want to throw on exception type that is a real RuntimeException, so that in Java things work as expected (only catch NakshaException and then react only to what you can handle).
| if (feature == null) { | ||
| val readFeatures = ReadFeatures(collection.id) | ||
| readFeatures.featureIds.add(featureId) | ||
| val response = PgReader(session, readFeatures).execute().proxy(SuccessResponse::class) |
There was a problem hiding this comment.
How about error response handling?
There was a problem hiding this comment.
Current design is that PgReader.readFeatures() will always return SuccessResponse, simply empty result if the features do not exist. But will add logic to skip tables modification if feature does not exist, simply return feature deleted
| @@ -123,10 +125,11 @@ class UpdateFeature( | |||
| } | |||
| } | |||
|
|
|||
| private fun insertPreviousVersionToHst( | |||
| protected fun insertHeadVersionToHst( | |||
There was a problem hiding this comment.
I'm okay with renaming the method, but then make the name more expressive. What the method does, is copyHeadToHistory, but what is wrong is versionInHst, the version actually stays unchanged (txn stays unchanged), what the method does is setting nextVersion, so, it copies from HEAD, setting txn_next which is next-version. So, if you rename, please be precise.
There was a problem hiding this comment.
The name is actually "insert HEAD version to Hst" so it is correct. But I will find better expressions to avoid this misunderstanding about version
There was a problem hiding this comment.
As said, I'm okay with the method name (even while I prefer mine ;-)), but the argument name is very misleading, please rename it back from versionInHst to nextVersion, because the version is not changed. However, this is only a minor issue, after all.
| @@ -137,8 +140,9 @@ class UpdateFeature( | |||
| sql = """ | |||
| INSERT INTO $hstTableName(${PgColumn.txn_next.name},$columnsWithoutNext) | |||
| SELECT $1,$columnsWithoutNext FROM $headTableName | |||
| WHERE $quotedIdColumn='$featureId' | |||
| """.trimIndent(), | |||
There was a problem hiding this comment.
This query is prone to SQL injections and must not stay like this. Please always use prepared statements with place-holders, except for identifiers (column- and table-names), where PostgresQL does not allow this. In other words, the query need to have two arguments and look like:
INSERT INTO $hstTableName(${PgColumn.txn_next}, $columnsWithoutNext) SELECT $1, $columnsWithoutNext FROM $headTableName WHERE ${PgColumn.id} = $2
here-naksha-lib-psql/src/commonMain/kotlin/naksha/psql/executors/write/InsertFeature.kt
Show resolved
Hide resolved
| import naksha.psql.executors.write.WriteFeatureUtils.tuple | ||
|
|
||
| class DeleteFeature(session: PgSession) : UpdateFeature(session) { | ||
| override fun execute(collection: PgCollection, write: Write): Tuple { |
There was a problem hiding this comment.
Please add a TODO, so that we're aware that this implementation currently do not support atomic updates! In other words, it ignores if version is set in the Write operation, which requires that the current HEAD state is exactly in this version.
| require(featureId != null) {"No feature ID provided"} | ||
|
|
||
| var feature = write.feature | ||
| if (feature == null) { |
There was a problem hiding this comment.
We should always fetch the current HEAD state, relying on the given one can be very dangerous. There is a helper method for this in PgStorage called getLatestTuples (maybe we should rename it into getHeadTuples and add a getHeadTuple as helper). It would be helpful to implement this, and use here (due to caching efficiency). I'm fine in just adding a TODO, so we know this code can be wrong. The thing is, we need to ensure that the feature exists in HEAD, before we try to delete it!
There was a problem hiding this comment.
getLatestTuples not yet implemented, so I opt to use PgReader for the time being
| } | ||
| } | ||
|
|
||
| val previousMetadata = fetchCurrentMeta(collection, featureId) |
There was a problem hiding this comment.
If we have no feature, then deleting is impossible. This should not trigger a failure, but we need to simply say, deletion is successful here (we can abort), because actually the feature is not in HEAD! It is a no go to continue processing with a missing feature!
| metadata(tupleNumber, feature, featureId, flags), | ||
| write.attachment, | ||
| flags | ||
| ) |
There was a problem hiding this comment.
For what do we need the tuple? When deleting, we first copy from HEAD to HISTORY, then we copy again from HEAD to HISTORY and/or DELETED (the tombstone). We do not need to create any new tuples our self, because no data is modified, only minor metadata is changed. The tuple-number for the first copy stays unchanged. The tuple-number of the second copy will only get a new txn (version), uid (unique-id in version) and flags. We only need to return the tuple-number, which we can generate without any further knowledge. Nothing else (as well not the hash) do change!
| @@ -80,7 +80,7 @@ internal object WriteFeatureUtils { | |||
| internal fun tuple( | |||
| storage: PgStorage, | |||
| tupleNumber: TupleNumber, | |||
| feature: NakshaFeature, | |||
| feature: NakshaFeature?, | |||
There was a problem hiding this comment.
This is not necessary, stick with feature being not null! See above in review why!
# Conflicts: # here-naksha-lib-psql/src/commonMain/kotlin/naksha/psql/executors/PgReader.kt
In this pull request: