Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add user types to support Array in Cloud SQL #439

Merged
merged 3 commits into from Jan 10, 2020

Conversation

@hstonec
Copy link
Collaborator

hstonec commented Jan 9, 2020

This PR added a series of Hibernate UserType classes which can be used to store/retrieve the collection of Java strings as array into Cloud SQL.

This change is Reviewable

@googlebot googlebot added the cla: yes label Jan 9, 2020
@hstonec hstonec requested review from weiminyu and mindhog Jan 9, 2020
@hstonec hstonec force-pushed the hstonec:add-array-support branch 2 times, most recently from 56cd4ac to 8377150 Jan 9, 2020
Copy link
Member

mindhog left a comment

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hstonec and @weiminyu)


core/src/main/java/google/registry/persistence/GenericCollectionUserType.java, line 61 at r1 (raw file):

      ResultSet rs, String[] names, SharedSessionContractImplementor session, Object owner)
      throws HibernateException, SQLException {
    if (rs != null && names != null && names.length > 0 && rs.getArray(names[0]) != null) {

I don't think the result set or names can be null, I think it's only null values coming from the database that you need to deal with.


core/src/main/java/google/registry/persistence/StringCollectionUserType.java, line 23 at r1 (raw file):

public abstract class StringCollectionUserType<T extends Collection<String>>
    extends GenericCollectionUserType<T> {
  public static final int COL_TYPE_CODE = Types.ARRAY;

It seems like these always have to be Types.ARRAY, right? Should the SQL type be moved into the base class?

Copy link
Collaborator Author

hstonec left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mindhog and @weiminyu)


core/src/main/java/google/registry/persistence/GenericCollectionUserType.java, line 61 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

I don't think the result set or names can be null, I think it's only null values coming from the database that you need to deal with.

Done.


core/src/main/java/google/registry/persistence/StringCollectionUserType.java, line 23 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

It seems like these always have to be Types.ARRAY, right? Should the SQL type be moved into the base class?

Types.ARRAY is the only type code provided by JDBC for array, which means there is no specific type code for text[], integer[], etc. Right now, we actually map all Types.ARRAY to text[], see the code change in NomulusPostgreSQLDialect. Later if we need support for integer[], we need to have a IntegerCollectionUserType and set COL_TYPE_CODE to something else and call registerColumnType(IntegerCollectoinUserType.COL_TYPE_CODE, IntegerCollectoinUserType.COL_DDL_TYPE_NAME); to register the new type.

Copy link
Member

mindhog left a comment

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @weiminyu)

Copy link
Collaborator

weiminyu left a comment

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hstonec)


core/src/main/java/google/registry/persistence/GenericCollectionUserType.java, line 35 at r2 (raw file):

supplier = getCollectionSupplier();

Calling overridden methods from constructor is anti-pattern. Make it a constructor parameter, which allows supplier to be final too.

You may also want to use checkNotNull here.


core/src/main/java/google/registry/persistence/GenericCollectionUserType.java, line 105 at r2 (raw file):

Quoted 24 lines of code…
  @Override
  public Object deepCopy(Object value) throws HibernateException {
    return value;
  }

  @Override
  public boolean isMutable() {
    return false;
  }

  @Override
  public Serializable disassemble(Object value) throws HibernateException {
    return (Serializable) value;
  }

  @Override
  public Object assemble(Serializable cached, Object owner) throws HibernateException {
    return cached;
  }

  @Override
  public Object replace(Object original, Object target, Object owner) throws HibernateException {
    return original;
  }

I think these methods provides safeguards to unconventional uses, e.g., modifying the collections after a save/merge call.
I'm not sure that they are implemented correctly: the java type we are dealing with are mutable.
At the minimum, we should add a TODO and open a bug to investigate.


core/src/main/java/google/registry/persistence/StringCollectionUserType.java, line 21 at r2 (raw file):

public abstract class StringCollectionUserType<T extends Collection<String>>

nit: I feel this class as an intermediate layer is not very useful. It'd be fine to:

  • move the static final fields to parent class as protected field
  • Have leaf classes pass sqlTypes and columnTypeName to base class.
@hstonec hstonec force-pushed the hstonec:add-array-support branch from 42262c0 to 13c99ea Jan 10, 2020
Copy link
Collaborator Author

hstonec left a comment

Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @mindhog and @weiminyu)


core/src/main/java/google/registry/persistence/GenericCollectionUserType.java, line 35 at r2 (raw file):

Previously, weiminyu (Weimin Yu) wrote…
supplier = getCollectionSupplier();

Calling overridden methods from constructor is anti-pattern. Make it a constructor parameter, which allows supplier to be final too.

You may also want to use checkNotNull here.

Refactored that part to avoid that kind of usage.


core/src/main/java/google/registry/persistence/GenericCollectionUserType.java, line 105 at r2 (raw file):

Previously, weiminyu (Weimin Yu) wrote…
  @Override
  public Object deepCopy(Object value) throws HibernateException {
    return value;
  }

  @Override
  public boolean isMutable() {
    return false;
  }

  @Override
  public Serializable disassemble(Object value) throws HibernateException {
    return (Serializable) value;
  }

  @Override
  public Object assemble(Serializable cached, Object owner) throws HibernateException {
    return cached;
  }

  @Override
  public Object replace(Object original, Object target, Object owner) throws HibernateException {
    return original;
  }

I think these methods provides safeguards to unconventional uses, e.g., modifying the collections after a save/merge call.
I'm not sure that they are implemented correctly: the java type we are dealing with are mutable.
At the minimum, we should add a TODO and open a bug to investigate.

I had the same feelings but the unit tests I added worked as expected. I added a TODO here for further investigation.


core/src/main/java/google/registry/persistence/StringCollectionUserType.java, line 21 at r2 (raw file):

Previously, weiminyu (Weimin Yu) wrote…
public abstract class StringCollectionUserType<T extends Collection<String>>

nit: I feel this class as an intermediate layer is not very useful. It'd be fine to:

  • move the static final fields to parent class as protected field
  • Have leaf classes pass sqlTypes and columnTypeName to base class.

Good point. Removed this layer and introduced an Enum to help manage the mapping.

@hstonec hstonec force-pushed the hstonec:add-array-support branch from 13c99ea to ef632f8 Jan 10, 2020
@hstonec hstonec force-pushed the hstonec:add-array-support branch from ef632f8 to e9e76af Jan 10, 2020
Copy link
Collaborator

weiminyu left a comment

Reviewed 2 of 7 files at r1, 5 of 5 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @weiminyu)

@hstonec hstonec merged commit 4eae108 into google:master Jan 10, 2020
6 of 7 checks passed
6 of 7 checks passed
code-review/reviewable 3 discussions left (weiminyu)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
cla/google All necessary CLAs are signed
kokoro-foss Kokoro build finished
Details
kokoro-internal Kokoro build finished
Details
@hstonec hstonec deleted the hstonec:add-array-support branch Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.