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

Feature/fdp 1628 #7

Merged
merged 14 commits into from
Dec 5, 2023
Merged

Feature/fdp 1628 #7

merged 14 commits into from
Dec 5, 2023

Conversation

LucianoFavoroso
Copy link
Contributor

No description provided.

Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Copy link
Contributor

@JelleHoffman JelleHoffman left a comment

Choose a reason for hiding this comment

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

There are still Sonar Encryption errors?

Copy link
Contributor

@JelleHoffman JelleHoffman left a comment

Choose a reason for hiding this comment

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

Sonar issues are false postives, therefore approved

class PskControllerTest {

@Autowired
private lateinit var mockMvc: MockMvc
Copy link
Contributor

Choose a reason for hiding this comment

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

Geen technische namen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deze vind ik lastig heb je een suggestie?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bijvoorbeeld pskRequest, dat is wat je uitvoert met dit object in de code

val expectedKey = "1234"
pskRepository.save(Psk(IDENTITY, Instant.MAX, expectedKey))

val currentPks = pskService.getCurrentPsk(IDENTITY)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: currentPsk, niet Pks

}

private fun constructSetPskCommand(key: String): String {
return "PSK:${key};PSK:${key}SET"
Copy link
Contributor

Choose a reason for hiding this comment

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

mag zonder { }

}

private fun generatePsk(): String {
val allowedChars = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
Copy link
Contributor

Choose a reason for hiding this comment

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

Deze kan ook als constant

@Test
fun shouldBeAbleTeEncryptAndDecryptData() {
val expected = "data"
val encrypted = databaseFieldEncryptor.convertToDatabaseColumn(expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestie: zet hier een extra assert tussen om aan tegeven dat encrypted != expected. Nu toont je test aan dat de encryptor z'n werk perfect doet of dat 'ie helemaal niks doet...

build.gradle.kts Outdated
@@ -39,7 +39,7 @@ subprojects {

group = "org.gxf.template"
version = rootProject.version

Copy link
Member

Choose a reason for hiding this comment

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

Remove whitespace

Comment on lines 10 to 28
implementation("org.springframework.boot:spring-boot-starter-actuator")
implementation("org.springframework.boot:spring-boot-starter-web")

implementation("org.jetbrains.kotlin:kotlin-reflect")
implementation("org.springframework:spring-aop")
implementation("io.github.microutils:kotlin-logging-jvm:3.0.5")

implementation("org.postgresql:postgresql:42.5.4")
implementation("org.flywaydb:flyway-core:9.22.3")

implementation("org.springframework.boot:spring-boot-starter-data-jpa:3.1.5")

implementation("org.springframework.boot:spring-boot-autoconfigure")
implementation("org.springframework.boot:spring-boot-starter-logging")

implementation("org.springframework:spring-aspects")
implementation("org.springframework:spring-aop")

implementation("org.springframework.kafka:spring-kafka")
implementation("com.microsoft.azure:msal4j:1.13.10")
Copy link
Member

Choose a reason for hiding this comment

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

  1. verify if the versions are not provided by spring boot starter(s)
  2. try to keep the dependencies organized in a logical manner.
  3. consider using a version catalogue for managing the dependencies.

Comment on lines 64 to 72
implementation("org.springframework.boot:spring-boot-starter-data-jpa:3.1.5")
implementation("com.gxf.utilities:kafka-avro:0.2")
implementation("org.springframework.kafka:spring-kafka")
implementation("org.springframework.boot:spring-boot-starter-test")
implementation("org.springframework.kafka:spring-kafka-test")
implementation("org.testcontainers:kafka:1.17.6")
implementation("org.springframework.ws:spring-ws-test")
runtimeOnly("com.h2database:h2:2.2.224")
implementation("org.mockito.kotlin:mockito-kotlin:5.1.0")
Copy link
Member

Choose a reason for hiding this comment

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

See previous comment regarding versions


@Test
fun shouldReturnKeyForIdentity() {
Mockito.`when`(mock.getCurrentPsk(any())).thenReturn("key")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Mockito.`when`(mock.getCurrentPsk(any())).thenReturn("key")
`when`(mock.getCurrentPsk(any())).thenReturn("key")


@Test
fun shouldReturn404WhenPskForIdentityIsNotFound() {
Mockito.`when`(mock.getCurrentPsk(any())).thenReturn(null)
Copy link
Member

Choose a reason for hiding this comment

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

Import static method, so that the code becomes:

Suggested change
Mockito.`when`(mock.getCurrentPsk(any())).thenReturn(null)
`when`(mock.getCurrentPsk(any())).thenReturn(null)

import org.gxf.crestdeviceservice.data.entity.Psk
import org.gxf.crestdeviceservice.psk.PskRepository
import org.gxf.crestdeviceservice.psk.PskService
import org.junit.jupiter.api.Assertions.*
Copy link
Member

Choose a reason for hiding this comment

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

Use AssertJ for assertions

Comment on lines 19 to 20
dev-initial-psk: "ABCDEFGHIJKLMNOP" #Should be the same as the PSK in the simulator only relevant for dev
dev-device-identity: "867787050253370" #Should be the same as the identity in the simulator only relevant for dev
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to have:

  • dev properties in a separate application-dev.yaml file
  • comments on a separate line:
Suggested change
dev-initial-psk: "ABCDEFGHIJKLMNOP" #Should be the same as the PSK in the simulator only relevant for dev
dev-device-identity: "867787050253370" #Should be the same as the identity in the simulator only relevant for dev
# Initial PSK should be the same as the PSK in the simulator, only relevant for dev
dev-initial-psk: "ABCDEFGHIJKLMNOP"
# Identity should be the same as the identity in the simulator, only relevant for dev
dev-device-identity: "867787050253370"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know what our plans regarding integration tests are and how spring profiles fit in. For now I suggest to keep it like this and update when needed. Comments on new line is done.


/***
* Setup function for local development.
* Runs flyway migrations manual, because the CommandLineRunner is run before spring runs the flyway migrations automatically.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Runs flyway migrations manual, because the CommandLineRunner is run before spring runs the flyway migrations automatically.
* Runs flyway migrations manually, because the CommandLineRunner is run before spring runs the flyway migrations automatically.

Comment on lines 30 to 32
val flyway = Flyway.configure()
.dataSource(dataSourceProperties.url, dataSourceProperties.username, dataSourceProperties.password)
.load()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val flyway = Flyway.configure()
.dataSource(dataSourceProperties.url, dataSourceProperties.username, dataSourceProperties.password)
.load()
val flyway = Flyway
.configure()
.dataSource(dataSourceProperties.url, dataSourceProperties.username, dataSourceProperties.password)
.load()

Copy link
Member

@smvdheijden smvdheijden left a comment

Choose a reason for hiding this comment

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

  • Add license headers!!!
  • Use AssertJ assertions

Comment on lines 10 to 28
implementation("org.springframework.boot:spring-boot-starter-actuator")
implementation("org.springframework.boot:spring-boot-starter-web")

implementation("org.jetbrains.kotlin:kotlin-reflect")
implementation("org.springframework:spring-aop")
implementation("io.github.microutils:kotlin-logging-jvm:3.0.5")

implementation("org.postgresql:postgresql:42.5.4")
implementation("org.flywaydb:flyway-core:9.22.3")

implementation("org.springframework.boot:spring-boot-starter-data-jpa:3.1.5")

implementation("org.springframework.boot:spring-boot-autoconfigure")
implementation("org.springframework.boot:spring-boot-starter-logging")

implementation("org.springframework:spring-aspects")
implementation("org.springframework:spring-aop")

implementation("org.springframework.kafka:spring-kafka")
implementation("com.microsoft.azure:msal4j:1.13.10")
Copy link
Member

Choose a reason for hiding this comment

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

  1. Verify for each dependency if the version is really needed or already supplied via Spring Boot
  2. Consider organizing the dependencies (alphabetically and/or grouped by feature?)
  3. Consider using a version catalogue for dependency management

Comment on lines 62 to 72
implementation(project())
implementation(project(":components:avro-measurement"))
implementation("org.springframework.boot:spring-boot-starter-data-jpa:3.1.5")
implementation("com.gxf.utilities:kafka-avro:0.2")
implementation("org.springframework.kafka:spring-kafka")
implementation("org.springframework.boot:spring-boot-starter-test")
implementation("org.springframework.kafka:spring-kafka-test")
implementation("org.testcontainers:kafka:1.17.6")
implementation("org.springframework.ws:spring-ws-test")
runtimeOnly("com.h2database:h2:2.2.224")
implementation("org.mockito.kotlin:mockito-kotlin:5.1.0")
Copy link
Member

Choose a reason for hiding this comment

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

See previous comment regarding dependencies


@Test
fun shouldReturn404WhenPskForIdentityIsNotFound() {
Mockito.`when`(mock.getCurrentPsk(any())).thenReturn(null)
Copy link
Member

Choose a reason for hiding this comment

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

Use static method imports for Mockito.`when`

Comment on lines 29 to 30
MockMvcRequestBuilders.get("/psk")
.header("x-device-identity", "identity"))
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer this layout for better readability.

Suggested change
MockMvcRequestBuilders.get("/psk")
.header("x-device-identity", "identity"))
MockMvcRequestBuilders
.get("/psk")
.header("x-device-identity", "identity"))

Comment on lines 19 to 20
dev-initial-psk: "ABCDEFGHIJKLMNOP" #Should be the same as the PSK in the simulator only relevant for dev
dev-device-identity: "867787050253370" #Should be the same as the identity in the simulator only relevant for dev
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dev-initial-psk: "ABCDEFGHIJKLMNOP" #Should be the same as the PSK in the simulator only relevant for dev
dev-device-identity: "867787050253370" #Should be the same as the identity in the simulator only relevant for dev
# Initial PSK should be the same as the PSK in the simulator, only relevant for dev
initial-psk: "ABCDEFGHIJKLMNOP"
# Device identity should be the same as the identity in the simulator, only relevant for dev
device-identity: "867787050253370"

Copy link
Member

Choose a reason for hiding this comment

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

Move class to package org.gxf.crestdeviceservice.coap

@@ -0,0 +1,40 @@
import org.gxf.crestdeviceservice.coap.DownlinkService
Copy link
Member

Choose a reason for hiding this comment

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

Import should not be needed when test class is in same package as class under test.

Comment on lines 24 to 34
/***
* Setup function for local development.
* Runs flyway migrations manual, because the CommandLineRunner is run before spring runs the flyway migrations automatically.
* Sets the psk used by the simulator if it doesn't exist.
*/
override fun run(vararg args: String?) {
val flyway = Flyway.configure()
.dataSource(dataSourceProperties.url, dataSourceProperties.username, dataSourceProperties.password)
.load()

flyway.migrate()
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried adding @DependsOn({"flyway", "flywayInitializer"}) annotation to the class?

Comment on lines 12 to 23
fun getDownlinkForIdentity(identity: String): String {

if (pskService.hasDefaultKey(identity)) {
logger.info { "Device $identity has default key creating new key " }

val newKey = pskService.generateAndSetNewKeyForIdentity(identity)

return constructSetPskCommand(newKey)
}

return "0"
}
Copy link
Member

Choose a reason for hiding this comment

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

Probably sufficient for now, but in the (near) future we might also have to handle other downlinks as well...
I'm wondering if it should be the responsibility of the downlink service to check and recreate PSKs...

Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>

val consumer = createKafkaConsumer(embeddedKafkaBroker, crestMessageTopicName)
val response = testRestTemplate.postForEntity("/sng/1", request, String::class.java)

Assertions.assertEquals("0", response.body)
assertThat("0").isEqualTo(response.body)
Copy link
Member

Choose a reason for hiding this comment

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

Wrong order of actual and expected values


val expectedJsonNode = ObjectMapper().readTree(getFileContentAsString("message.json"))
val payloadJsonNode = ObjectMapper().readTree(records.records(crestMessageTopicName).first().value().payload)

Assertions.assertEquals(expectedJsonNode, payloadJsonNode)
assertThat(expectedJsonNode).isEqualTo(payloadJsonNode)
Copy link
Member

Choose a reason for hiding this comment

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

Wrong order of actual and expected values

import org.testcontainers.shaded.com.fasterxml.jackson.databind.ObjectMapper
import java.time.Duration

@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@EmbeddedKafka(
topics = ["\${crest-device-service.kafka.message-producer.topic-name}"],
)
@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD)
class MessageHandelingTest {
Copy link
Member

Choose a reason for hiding this comment

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

Fix typo, also rename file.

Suggested change
class MessageHandelingTest {
class MessageHandlingTest {

val result = restTemplate.exchange("/psk",
HttpMethod.GET, HttpEntity<Unit>(headers), String::class.java)

assertThat("0000111122223333").isEqualTo(result.body)
Copy link
Member

Choose a reason for hiding this comment

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

Switch actual and expected values.

@@ -24,7 +25,7 @@ class DownlinkServiceTest {

val result = downLinkService.getDownlinkForIdentity("identity")

assertEquals("0", result)
assertThat("0").isEqualTo(result)
Copy link
Member

Choose a reason for hiding this comment

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

Switch actual and expected values

@@ -35,6 +36,6 @@ class DownlinkServiceTest {
val result = downLinkService.getDownlinkForIdentity("identity")

// Psk command is formatted as: PSK:[Key];PSK:[Key]SET
assertEquals("PSK:key;PSK:keySET", result)
assertThat("PSK:key;PSK:keySET").isEqualTo(result)
Copy link
Member

Choose a reason for hiding this comment

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

Switch actual and expected values

Comment on lines 16 to 17
assertThat("data").isNotEqualTo(encrypted)
assertThat(expected).isEqualTo(decrypted)
Copy link
Member

Choose a reason for hiding this comment

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

Switch actual and expected values.

Suggested change
assertThat("data").isNotEqualTo(encrypted)
assertThat(expected).isEqualTo(decrypted)
assertThat(encrypted).isNotEqualTo("data")
assertThat(decrypted).isEqualTo(expected)


library("microsoftMsal", "com.microsoft.azure", "msal4j").version("1.13.10")

library("assertJ", "org.assertj", "assertj-core").version("3.24.2")
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the version provided by Spring Boot?

library("kafkaTestContainers", "org.testcontainers", "kafka").version("1.17.6")
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Add newline

Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
# Conflicts:
#	application/src/test/kotlin/MeasurementProducerTest.kt
Signed-off-by: Luciano Favoroso <luciano.favoroso@alliander.com>
Copy link

sonarcloud bot commented Dec 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

77.0% 77.0% Coverage
0.0% 0.0% Duplication

@smvdheijden smvdheijden merged commit 45aa4dc into main Dec 5, 2023
4 checks passed
@smvdheijden smvdheijden deleted the feature/FDP-1628 branch December 5, 2023 13:16
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.

None yet

4 participants