Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,14 @@
import java.nio.BufferUnderflowException;
import java.nio.ByteBuffer;
import net.jcip.annotations.ThreadSafe;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

@ThreadSafe
public class UdtCodec implements TypeCodec<UdtValue> {

private static final Logger LOG = LoggerFactory.getLogger(UdtCodec.class);

private final UserDefinedType cqlType;

public UdtCodec(@NonNull UserDefinedType cqlType) {
Expand Down Expand Up @@ -107,10 +111,8 @@ public UdtValue decode(@Nullable ByteBuffer bytes, @NonNull ProtocolVersion prot
int i = 0;
while (input.hasRemaining()) {
if (i == cqlType.getFieldTypes().size()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels ok given that the only operations you can do on ALTER TYPE are ADD and RENAME,

Given that:

  1. you can't drop a udt field;
  2. adding a field is always additive (e.g. if you have existing fields, any added fields should come after, at least that is what it looks like this does: https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/cql3/statements/schema/AlterTypeStatement.java#L161-L162)

This means you won't risk reading the wrong field as position-ally everything should be retained after any ALTER TYPE operation.

Also considering that you can still write a udt value with the old udtCodec when the type is altered, it seems reasonable that you'd be able to read partially from it as well.

I suppose there is some inherent risk of someone creating a UdtCodec by hand and potentially reading the wrong interpretation of things; but I think that should probably done at your own peril anyways.

If you pull a UdtCodec from the registry and the type is later changed, the codec would still be usable after this change, this seems of more value than protecting against the risk I outlined.

throw new IllegalArgumentException(
String.format(
"Too many fields in encoded UDT value, expected %d",
cqlType.getFieldTypes().size()));
LOG.debug("Encountered unexpected fields when parsing codec {}", cqlType);
break;
}
int elementSize = input.getInt();
ByteBuffer element;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,18 +136,18 @@ public void should_decode_udt() {
}

@Test
public void should_fail_to_decode_udt_when_too_many_fields() {
assertThatThrownBy(
() ->
decode(
"0x"
+ ("00000004" + "00000001")
+ "ffffffff"
+ ("00000001" + "61")
// extra contents
+ "ffffffff"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Too many fields in encoded UDT value, expected 3");
public void should_decode_udt_when_too_many_fields() {
UdtValue udt =
decode(
"0x"
+ ("00000004" + "00000001")
+ "ffffffff"
+ ("00000001" + "61")
// extra contents
+ "ffffffff");
assertThat(udt.getInt(0)).isEqualTo(1);
assertThat(udt.isNull(1)).isTrue();
assertThat(udt.getString(2)).isEqualTo("a");
}

/** Test for JAVA-2557. Ensures that the codec can decode null fields with any negative length. */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.datastax.oss.driver.internal.core.type.codec;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import com.datastax.oss.driver.api.core.CqlSession;
import com.datastax.oss.driver.api.core.cql.Row;
import com.datastax.oss.driver.api.core.data.UdtValue;
import com.datastax.oss.driver.api.core.type.UserDefinedType;
import com.datastax.oss.driver.api.core.type.codec.TypeCodec;
import com.datastax.oss.driver.api.testinfra.ccm.CcmRule;
import com.datastax.oss.driver.api.testinfra.session.SessionRule;
import com.datastax.oss.driver.categories.ParallelizableTests;
import java.util.Objects;
import org.junit.Rule;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.junit.rules.RuleChain;
import org.junit.rules.TestRule;

@Category(ParallelizableTests.class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reformat this with?:

mvn fmt:format

public class UdtCodecIT {

private CcmRule ccmRule = CcmRule.getInstance();

private SessionRule<CqlSession> sessionRule = SessionRule.builder(ccmRule).build();

@Rule public TestRule chain = RuleChain.outerRule(ccmRule).around(sessionRule);

@Test
public void should_decoding_udt_be_backward_compatible() {
CqlSession session = sessionRule.session();
session.execute("CREATE TYPE test_type_1 (a text, b int)");
session.execute("CREATE TABLE test_table_1 (e int primary key, f frozen<test_type_1>)");
// insert a row using version 1 of the UDT schema
session.execute("INSERT INTO test_table_1(e, f) VALUES(1, {a: 'a', b: 1})");
UserDefinedType udt =
session
.getMetadata()
.getKeyspace(sessionRule.keyspace())
.flatMap(ks -> ks.getUserDefinedType("test_type_1"))
.orElseThrow(IllegalStateException::new);
TypeCodec<?> oldCodec = session.getContext().getCodecRegistry().codecFor(udt);
// update UDT schema
session.execute("ALTER TYPE test_type_1 add i text");
// insert a row using version 2 of the UDT schema
session.execute("INSERT INTO test_table_1(e, f) VALUES(2, {a: 'b', b: 2, i: 'b'})");
Row row =
Objects.requireNonNull(session.execute("SELECT f FROM test_table_1 WHERE e = ?", 2).one());
// Try to read new row with old codec. Using row.getUdtValue() would not cause any issues,
// because new codec will be automatically registered (using all 3 attributes).
// If application leverages generic row.get(String, Codec) method, data reading with old codec
// should
// be backward-compatible.
UdtValue value = Objects.requireNonNull((UdtValue) row.get("f", oldCodec));
assertThat(value.getString("a")).isEqualTo("b");
assertThat(value.getInt("b")).isEqualTo(2);
assertThatThrownBy(() -> value.getString("i")).hasMessage("i is not a field in this UDT");
}
}