fix: STREAMP-12336: removing validation on schema key if it's changing in update request #361
Conversation
| client.getOptionalData(factory.upsertStreamMutationBuilder() | ||
| .schema(nonExisting) | ||
| .build()).get(); | ||
| } catch (RuntimeException ex) { |
There was a problem hiding this comment.
Doing this validation from egsp-stream-reg for update case.
| @@ -82,7 +82,9 @@ public Optional<Stream> update(Stream stream) throws ValidationException { | |||
| if (existing.isEmpty()) { | |||
| throw new ValidationException("Can't update " + stream.getKey() + " because it doesn't exist"); | |||
| } | |||
| stream.setSchemaKey(existing.get().getSchemaKey()); | |||
| if (stream.getSchemaKey() == null) { | |||
There was a problem hiding this comment.
this check for SMS, if update request not having schema .
sunlnu
changed the title
| if (stream.getSchemaKey() == null) { | ||
| throw new ValidationException("Schema must be specified"); | ||
| } | ||
| } |
There was a problem hiding this comment.
What is the goal of the changes in these validations? As far as I can tell all that has been changed is that previously, on update a schema key was required to be present and the schema exists. But now, it seems like on update the schema is required to be present, but is NOT required to exist? I don't think this is desirable?
There was a problem hiding this comment.
if (!schemaView.exists(stream.getSchemaKey())) {
throw new ValidationException("Schema does not exist");
}
this validation i have removed from update , this was creating issue, if customers changes the schemaKey.
| Stream existingEntity = new Stream(key, new Specification(), new SchemaKey("domain", "stream_v1")); | ||
| Stream updatedEntity = new Stream(key, new Specification(), new SchemaKey("domain", "stream_v2")); | ||
| when(streamRepository.findById(key)).thenReturn(Optional.of(existingEntity)); | ||
| streamService.update(updatedEntity); |
There was a problem hiding this comment.
Should this throw an error?
There was a problem hiding this comment.
yeah i have added this
@Test(expected = ValidationException.class)
do you want me to add error message ??
There was a problem hiding this comment.
now asserting the exact error .
| Stream existingEntity = new Stream(key, new Specification(), new SchemaKey("domain", "stream_v1")); | ||
| Stream updatedEntity = new Stream(key, new Specification(), null); | ||
| when(streamRepository.findById(key)).thenReturn(Optional.of(existingEntity)); | ||
| streamService.update(updatedEntity); |
There was a problem hiding this comment.
can we validate the output contains the non null schemakey
| @@ -1,5 +1,5 @@ | |||
| /** | |||
| * Copyright (C) 2018-2021 Expedia, Inc. | |||
There was a problem hiding this comment.
Are changes to this file still necessary?
There was a problem hiding this comment.
yeah because now, we have check if schema is not matching with existing schemaKey.
stream-registry PR
<So what happens currently, if customer want to change the SchemaKey for a stream, we are allowing to do this.
Although we are not updating schemaKey and in control-plane stream still pointing to older schema.
Probably, we should not allow to customer to change the schemaKey for a stream, and give the valid error if any customer trying to do.
For now plan is, not throw error from OOS project, in place of we will throw error from egsp-stream-registry.
>
Added
Changed
Deleted
PR Checklist Forms