Skip to content

Refactor EppResourceUtils.loadByForeignKey() -> ForeignKeyUtils.loadResource()#2864

Merged
CydeWeys merged 1 commit into
google:masterfrom
CydeWeys:fku-loadresource
Oct 28, 2025
Merged

Refactor EppResourceUtils.loadByForeignKey() -> ForeignKeyUtils.loadResource()#2864
CydeWeys merged 1 commit into
google:masterfrom
CydeWeys:fku-loadresource

Conversation

@CydeWeys

@CydeWeys CydeWeys commented Oct 28, 2025

Copy link
Copy Markdown
Member

This doesn't make any underlying implementation details, and is mainly useful to reduce the number of diffs in PR #2852 (which does change implementation details) thus making that easier to review.


This change is Reviewable

@gbrodman gbrodman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@gbrodman reviewed 36 of 36 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CydeWeys)


core/src/main/java/google/registry/model/EppResourceUtils.java line 132 at r1 (raw file):

  }

  static <T extends EppResource> Optional<T> loadByForeignKeyHelper(

this is kind of an odd halfway state, should we (in this PR) move this method and the two methods above (loadByForeignKeysByCacheIfEnabled and loadByForeignKeysByCache) to ForeignKeyUtils so that everything can live there?

@CydeWeys CydeWeys left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/main/java/google/registry/model/EppResourceUtils.java line 132 at r1 (raw file):

Previously, gbrodman wrote…

this is kind of an odd halfway state, should we (in this PR) move this method and the two methods above (loadByForeignKeysByCacheIfEnabled and loadByForeignKeysByCache) to ForeignKeyUtils so that everything can live there?

I'm OK with odd halfway states so long as nothing breaks. I just wanted to make this PR simple, with one method refactor, and not include anything else you'd need to undo in the subsequent one.

@gbrodman gbrodman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @CydeWeys)


core/src/main/java/google/registry/model/EppResourceUtils.java line 132 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I'm OK with odd halfway states so long as nothing breaks. I just wanted to make this PR simple, with one method refactor, and not include anything else you'd need to undo in the subsequent one.

eh fair, i think a large part of why the other PR is kinda big is that I was trying to avoid halfway states

…adResource(...)

This doesn't make any underlying implementation details, and is mainly useful to
reduce the number of diffs in PR google#2852 (which does change implementation
details) thus making that easier to review.

@CydeWeys CydeWeys left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @CydeWeys)


core/src/main/java/google/registry/model/EppResourceUtils.java line 132 at r1 (raw file):

Previously, gbrodman wrote…

eh fair, i think a large part of why the other PR is kinda big is that I was trying to avoid halfway states

They're fine, just a part of doing http://go/small-cls . The halfway state shouldn't even last more than a day or two anyway.

@gbrodman gbrodman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @CydeWeys)

@CydeWeys
CydeWeys added this pull request to the merge queue Oct 28, 2025
Merged via the queue into google:master with commit ad07b32 Oct 28, 2025
9 checks passed
@CydeWeys
CydeWeys deleted the fku-loadresource branch October 28, 2025 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants