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

Fix NPE in CrudRepositoryExtensions #3187

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

OsaSoft
Copy link

@OsaSoft OsaSoft commented Oct 24, 2024

Erasure of nullability information was causing NPEs in the forced non-null assertion, as it allowed to pass a nullable value, see following snippet that would pass compilation correctly, but throw an unexpected NPE:

interface Repo : ListCrudRepository<User, String>

	val repo: Repo = mockk()

	@Test
	fun `CrudRepository#findByIdOrNull() extension should not throw NPE`() {
		val id: String? = null
		// compiler allows null value to be passed to findByIdOrNull, so NPE is thrown
		// I believe this is we lose the non-null type info when the repo extends a java interface,
		// which extends CrudRepository
		repo.findByIdOrNull(id)
	}

The fix enforces the generic ID type to be a not-null type, therefore the above code no longer compiles.
image

Not sure how to add a test for this, since the fix is on a static-type level and the problematic code no longer compiles. Could add in a comment, I guess?

@pivotal-cla
Copy link

@OsaSoft Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 24, 2024
@pivotal-cla
Copy link

@OsaSoft Thank you for signing the Contributor License Agreement!

@mp911de mp911de self-assigned this Oct 24, 2024
@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 24, 2024
@mp911de
Copy link
Member

mp911de commented Oct 24, 2024

The type should be non-nullable indeed. We would incorporate this change for the next GA release only without backporting it as this is a breaking change in some sense regarding compiling.

@mp911de mp911de added the status: on-hold We cannot start working on this issue yet label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: on-hold We cannot start working on this issue yet type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants