Skip to content
This repository was archived by the owner on Apr 7, 2026. It is now read-only.

fix: fixed visibility of LazyInit methods #461

Closed
olavloite wants to merge 2 commits into from
Closed

fix: fixed visibility of LazyInit methods #461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3dbec10
fix: fixed visibility of LazyInit methods
olavloite Sep 29, 2020
cc7f10e
fix: add ignored diff
olavloite Sep 29, 2020
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
6 changes: 6 additions & 0 deletions google-cloud-spanner/clirr-ignored-differences.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,4 +319,10 @@
<className>com/google/cloud/spanner/Value</className>
<method>java.util.List getNumericArray()</method>
</difference>

<difference>
<differenceType>7009</differenceType>
<className>com/google/cloud/spanner/AbstractLazyInitializer</className>
<method>* initialize()</method>
</difference>
</differences>
4 changes: 2 additions & 2 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractLazyInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public abstract class AbstractLazyInitializer<T> {
private volatile Exception error;

/** Returns an initialized instance of T. */
T get() throws Exception {
public T get() throws Exception {
// First check without a lock to improve performance.
if (!initialized) {
synchronized (lock) {
Expand All @@ -51,5 +51,5 @@ T get() throws Exception {
* Initializes the actual object that should be returned. Is called once the first time an
* instance of T is required.
*/
public abstract T initialize() throws Exception;
protected abstract T initialize() throws Exception;

@elharo elharo Sep 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's probably too late to make this change without a major version bump.

@olavloite olavloite Sep 29, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I agree that it would normally require a major version bump to make such a change. I would however argue that in this specific case we should consider relaxing that requirement for the following reasons:

  • The current implementation marks the get() method as package-private, which means that the class can currently only be (purposefully) used by the client library itself, as any outside user would not be able to get an instance of the object that is created.
  • Reducing the visibility of the initialize() method would break any application that is directly calling the method, but that is not the intention of the method (and would also make no sense).
  • Reducing the visibility of the initialize() method does not break any application that extends the class and overrides this method using public visibility, as increasing the visibility of an overridden method is allowed.

So I would say that the probability of this change actually breaking source compatibility with any users is negligent. The change obviously does break binary compatibility.

All that being said: I would rather drop this specific visibility change than do a major version bump just for this. So if my arguments are not convincing enough (which I can perfectly understand from an API maintenance perspective), I'll revert this part of the change.

@elharo elharo Sep 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The get method change is fine. As you point out, it's OK to increase visibility. If that's sent separately, I'm fine with that much.

}