Skip to content

#31 #44

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

#31 #44

wants to merge 4 commits into from

Conversation

PavelRyauzov
Copy link

@PavelRyauzov PavelRyauzov commented Jun 21, 2025

closes #31


PR-Codex overview

This PR focuses on migrating Java code to Kotlin, enhancing the codebase's conciseness and readability while maintaining functionality. It also introduces a new XRolesAuthenticationFilter and updates endpoint handling with Kotlin's syntax.

Detailed summary

  • Deleted XRolesAuthenticationFilter.java.
  • Migrated TestApplication.java to TestApplication.kt.
  • Migrated Endpoints.java to Endpoints.kt, changing method signatures and authorization logic.
  • Migrated Config.java to Config.kt, simplifying security configuration.
  • Added XRolesAuthenticationFilter.kt with custom filtering logic.
  • Migrated EndpointsTest.java to EndpointsTest.kt, updating test method signatures and assertions.
  • Updated pom.xml to include Kotlin dependencies and configuration.
  • Changed the source directory for Kotlin files in pom.xml.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@PavelRyauzov PavelRyauzov changed the title feat(#31): migrate to kotlin #31 Jun 21, 2025
@PavelRyauzov
Copy link
Author

@l3r8yJ could you review this?

Copy link
Owner

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

@PavelRyauzov take a look, please

Copy link
Owner

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

@PavelRyauzov a few more


@SpringBootApplication
public class TestApplication {
open class TestApplication
Copy link
Owner

Choose a reason for hiding this comment

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

@PavelRyauzov open here is redundant, apply plugin to all classes, please

Copy link
Author

Choose a reason for hiding this comment

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

I was inattentive, fixed it

Comment on lines 44 to 46
val registration = FilterRegistrationBean(XRolesAuthenticationFilter())
registration.order = SecurityProperties.DEFAULT_FILTER_ORDER
return registration
Copy link
Owner

Choose a reason for hiding this comment

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

@PavelRyauzov try this

return FilterRegistrationBean(XRolesAuthenticationFilter()).apply {
    this.order = SecurityProperties.DEFAULT_FILTER_ORDER
}

Copy link
Author

Choose a reason for hiding this comment

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

cool, it looks more elegant, fixed this

class XRolesAuthenticationFilter : OncePerRequestFilter() {

private companion object {
private const val X_ROLES_HEADER = "X-Roles"
Copy link
Owner

Choose a reason for hiding this comment

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

@PavelRyauzov when companion object is private there is no need to make it members private, as far as i know. Double check this one please

Copy link
Author

Choose a reason for hiding this comment

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

I checked, you were right, fixed this

Comment on lines 49 to 53
val header = request.getHeader(X_ROLES_HEADER)
header ?: run {
chain.doFilter(request, response)
return
}
Copy link
Owner

Choose a reason for hiding this comment

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

@PavelRyauzov try this

val header = request.getHeader(X_ROLES_HEADER) ?: run { chain.doFilter(request, response); return }

Copy link
Author

Choose a reason for hiding this comment

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

done

Comment on lines 64 to 68
val authorized = SecurityContextHolder.getContext().authentication
authorized ?: run {
chain.doFilter(request, response)
return
}
Copy link
Owner

Choose a reason for hiding this comment

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

@PavelRyauzov same here

Copy link
Author

Choose a reason for hiding this comment

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

done

authorized.credentials,
authorities
)
SecurityContextHolder.getContext().authentication = withXRoles
Copy link
Owner

Choose a reason for hiding this comment

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

@PavelRyauzov try this

SecurityContextHolder.getContext().apply {
    this.authentication = withXRoles
 }

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Author

Choose a reason for hiding this comment

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

@l3r8yJ please look again

@l3r8yJ l3r8yJ self-requested a review June 21, 2025 17:01
Copy link
Owner

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

lgtm now

@l3r8yJ
Copy link
Owner

l3r8yJ commented Jun 21, 2025

@PavelRyauzov fix CI please, run xcop locally

@l3r8yJ
Copy link
Owner

l3r8yJ commented Jul 6, 2025

@PavelRyauzov some news?

@PavelRyauzov
Copy link
Author

@l3r8yJ done, fixed xml

@@ -62,6 +62,8 @@ SOFTWARE.
</ciManagement>
<properties>
<java.version>17</java.version>
<kotlin.version>1.9.22</kotlin.version>
Copy link
Owner

Choose a reason for hiding this comment

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

@PavelRyauzov lets use latest version of Kotlin?

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.

Rewrite it to kotlin
2 participants