Repository navigation
Conversation
| Right (RegisteredSchema schema, schemaId) -> cacheSchema sr schemaId schema $> Right schema | ||
| Right (RegisteredSchema schema, schemaId) -> do | ||
| cacheId sr subject (fullTypeName schema) schemaId | ||
| cacheSchema sr schemaId schema |
There was a problem hiding this comment.
The lack of id caching seems to have been an existing bug, since getCompatibleSchema expects to look up by id after calling loadSubjectSchema.
| let url = srBaseUrl sr ++ "/subjects/" ++ unpack sbj ++ "/versions/" ++ show version | ||
| loadSubjectSchema registry subject version = fmap snd <$> loadSubjectSchema' registry subject (SpecificVersion version) | ||
|
|
||
| loadSubjectSchema' :: MonadIO m => SchemaRegistry -> Subject -> QueryVersion -> m (Either SchemaRegistryError (SchemaId, Schema)) |
There was a problem hiding this comment.
Modifying loadSubjectSchema would be a breaking change, so I left this improved version "primed" and hidden for now. It could perhaps replace the original in a future major version.
There was a problem hiding this comment.
I'm not a maintainer of this package.
You should probably ping @AlexeyRaga for review.
But I do have some suggestions...
| newtype Version = Version { unVersion :: Word32 } deriving (Eq, Ord, Show, Hashable) | ||
| data QueryVersion = SpecificVersion Version | LatestVersion | ||
| renderQueryVersion :: QueryVersion -> String | ||
| renderQueryVersion = \case | ||
| SpecificVersion (Version version) -> show version | ||
| LatestVersion -> "latest" |
There was a problem hiding this comment.
quibble:
| newtype Version = Version { unVersion :: Word32 } deriving (Eq, Ord, Show, Hashable) | |
| data QueryVersion = SpecificVersion Version | LatestVersion | |
| renderQueryVersion :: QueryVersion -> String | |
| renderQueryVersion = \case | |
| SpecificVersion (Version version) -> show version | |
| LatestVersion -> "latest" | |
| newtype Version = Version { unVersion :: Word32 } deriving (Eq, Ord, Show, Hashable) | |
| data QueryVersion = SpecificVersion Version | LatestVersion | |
| renderQueryVersion :: QueryVersion -> String | |
| renderQueryVersion = \case | |
| SpecificVersion (Version version) -> show version | |
| LatestVersion -> "latest" |
| -- | When auto-registration of schemas is disabled, pick the latest version instead of using compatibility testing. | ||
| -- This is similar to the Confluent 'use.latest.version' option. | ||
| cfgUseLatestSchemaVersion :: Bool -> SchemaRegistryConfig -> SchemaRegistryConfig | ||
| cfgUseLatestSchemaVersion useLatestSchemaVersion config = config { cUseLatestSchemaVersion = useLatestSchemaVersion } | ||
|
|
There was a problem hiding this comment.
suggestion:
Adding this config option adds some complexity to the UX.
It's possible to set cfgAutoRegisterSchemas = True and cfgUseLatestSchemaVersion = True - which is a conflict. And I don't think Haddock explaining that cfgAutoRegisterSchemas has higher priority improves the UX.
I'd suggest to remove this config option. Instead, in the case where cfgAutoRegisterSchemas == False:
- Check if the latest schema version is compatible.
- If not, fall back to
getCompatibleSchema.
There was a problem hiding this comment.
Thanks for the feedback! The point of the option is to bypass compatibility testing in cases when you don't have permission, so a check with fallback unfortunately won't work. I realise that the auto registration option will take precedence, but that's also the case with the official Confluent library, and I figured it was best if the behaviour is similar. Otherwise, another option would've been to merge the two options and use a three-way sum type instead of a boolean, but that breaks compatibility.
There was a problem hiding this comment.
Right 😅
Personally, if it improves the UX, I'd prefer a sum type even if it's a breaking change (After all, PVP and SemVer exist to reduce the pain of dealing with breaking changes - and this package hasn't been very actively maintained lately anyway).
But that's not up to decide, so I'd suggest to wait for feedback from @AlexeyRaga.
|
Sorry, I commented in details here, but to me it looks like that the whole thing about using arbitrary schema IDs instead of registering real schemas doesn't look right. We should stop and rethink the matter. |
Fixes #60. @mrcjkb