Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 25 additions & 0 deletions src/main/java/com/bravo/user/controller/AddressController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package com.bravo.user.controller;

import com.bravo.user.annotation.SwaggerController;
import com.bravo.user.model.dto.AddressDto;
import com.bravo.user.service.AddressService;
import lombok.RequiredArgsConstructor;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.ResponseBody;

import java.util.List;

@RequiredArgsConstructor
@RequestMapping(value = "/user")
@SwaggerController
public class AddressController {
private final AddressService addressService;

@ResponseBody
@GetMapping(value = "/{userId}/address")
List<AddressDto> findAll(@PathVariable String userId) {
return addressService.findAllByUserId(userId);
}
}
10 changes: 10 additions & 0 deletions src/main/java/com/bravo/user/dao/repository/AddressRepository.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.bravo.user.dao.repository;

import com.bravo.user.dao.model.Address;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
import org.springframework.stereotype.Repository;

@Repository
public interface AddressRepository extends JpaRepository<Address, String>, JpaSpecificationExecutor<Address> {
Copy link
Member

Choose a reason for hiding this comment

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

JpaSpecificationExecutor is not really required here since we're only querying on a single input.

 List<Address> findByUserId(final String userId);

https://docs.spring.io/spring-data/jpa/docs/current/reference/html/#repositories.core-concepts

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeah that's right. I'm rusty there it seems. Last time I had to add a method to a repository interface was in 2015, or 7 years ago

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package com.bravo.user.dao.specification;

import com.bravo.user.dao.model.Address;
import com.bravo.user.model.filter.AddressFilter;
import lombok.RequiredArgsConstructor;

import javax.persistence.criteria.CriteriaBuilder;
import javax.persistence.criteria.CriteriaQuery;
import javax.persistence.criteria.Root;

@RequiredArgsConstructor
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using @RequiredArgsConstructor instead of @AllArgsConstructor?

Copy link
Author

Choose a reason for hiding this comment

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

I just use it by default, not much of a difference here I don't think. In case some one comes along and forgets to make their fields final maybe this can sorta catch that.

public class AddressSpecification extends AbstractSpecification<Address> {
private final AddressFilter filter;

@Override
void doFilter(Root<Address> root, CriteriaQuery<?> criteriaQuery, CriteriaBuilder criteriaBuilder) {
applyStringFilter(root.get("userId"), filter.getUserId());
}
}
12 changes: 12 additions & 0 deletions src/main/java/com/bravo/user/model/filter/AddressFilter.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.bravo.user.model.filter;

import lombok.Builder;
import lombok.Data;

import java.util.Set;

@Data
@Builder
public class AddressFilter {
private Set<String> userId;
}
29 changes: 29 additions & 0 deletions src/main/java/com/bravo/user/service/AddressService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package com.bravo.user.service;

import com.bravo.user.dao.model.Address;
import com.bravo.user.dao.model.mapper.ResourceMapper;
import com.bravo.user.dao.repository.AddressRepository;
import com.bravo.user.dao.specification.AddressSpecification;
import com.bravo.user.model.dto.AddressDto;
import com.bravo.user.model.filter.AddressFilter;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;

import java.util.List;
import java.util.Set;

@RequiredArgsConstructor
@Service
public class AddressService {
private final AddressRepository addressRepository;
private final ResourceMapper resourceMapper;

public List<AddressDto> findAllByUserId(String userId) {
AddressFilter filter = AddressFilter.builder()
.userId(Set.of(userId))
.build();
AddressSpecification specification = new AddressSpecification(filter);
List<Address> addresses = addressRepository.findAll(specification);
Copy link
Member

Choose a reason for hiding this comment

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

userId, specification and addresses can be final. Do you know the benefits of making these variables final?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I used to have that generator box checked too, but nowadays I think if someone have bugs from reassigning variables, then they probably have a bigger problem. Generally variables don't ever get reassigned anyway so there's no need to have final all over the place. I think just having them on property fields should be enough, maybe occasionally parameters if it's some logic heavy method.

return resourceMapper.convertAddresses(addresses);
}
}
49 changes: 49 additions & 0 deletions src/test/java/com/bravo/user/controller/AddressControllerTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package com.bravo.user.controller;

import com.bravo.user.model.dto.AddressDto;
import com.bravo.user.service.AddressService;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.test.web.servlet.MockMvc;

import java.util.List;

import static org.mockito.Mockito.when;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

@WebMvcTest(controllers = AddressController.class)
class AddressControllerTest {
@Autowired
private MockMvc mockMvc;
@MockBean
AddressService service;
Copy link
Member

Choose a reason for hiding this comment

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

service should be private.

Copy link
Author

Choose a reason for hiding this comment

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

Agree.


@Test
void getShouldReturnAddressWithMatchingZip() throws Exception {
AddressDto address = new AddressDto();
address.setZip("test zip");
AddressDto address1 = new AddressDto();
address1.setZip("test zip 1");
when(service.findAllByUserId("testId")).thenReturn(List.of(address, address1));
mockMvc.perform(get("/user/testId/address"))
.andExpect(status().isOk())
.andExpect(jsonPath("$[0].zip").value("test zip"))
.andExpect(jsonPath("$[1].zip").value("test zip 1"));
}

@Test
void emptyIdShouldNotFindAnything() throws Exception {
mockMvc.perform(get("/user/address")).andExpect(status().isNotFound());
}

@Test
void noDataShouldReturnEmptyList() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

These test names are not very clear. It would quickly get confusing if more endpoints were added to the AddressController.

Copy link
Author

Choose a reason for hiding this comment

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

I typically use subjectMethodNameShouldBehaveThisWay as a pattern, so in this case it's enough, and yes if more endpoints get in here then it'll have to be updated.

mockMvc.perform(get("/user/test id/address"))
.andExpect(status().isOk())
.andExpect(jsonPath("$").isEmpty());
}
}
47 changes: 47 additions & 0 deletions src/test/java/com/bravo/user/service/AddressServiceTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package com.bravo.user.service;

import com.bravo.user.dao.model.Address;
import com.bravo.user.dao.model.mapper.ResourceMapper;
import com.bravo.user.dao.repository.AddressRepository;
import com.bravo.user.model.dto.AddressDto;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.data.jpa.domain.Specification;

import java.util.List;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

@SpringBootTest
@ExtendWith(MockitoExtension.class)
class AddressServiceTest {
private AddressService testObj;
@Mock
private AddressRepository repository;
@Autowired
private ResourceMapper mapper;
Copy link
Member

Choose a reason for hiding this comment

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

  • @SpringBootTest is unncessary here.
  • You can use @InjectMocks to initialize AddressService.
  • You should use a better name to refer to the AddressService variable. It's unclear what testObj is without looking at the variable type.
  • Mock the ResourceMapper. We don't want ResourceMapper bugs to cause failures in AddressServiceTest.

Copy link
Author

Choose a reason for hiding this comment

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

Sometimes a field gets mocked but don't get called in a when(), then intellij says that variable is unused, which is not true but it messes with you when you're reading it.

With the mapper I think it being a utility component it's fine to have additional coverage on it, also gets rid of the need to do another when()

I call all subjectUnderTest testObj so that way it's quick and easy to see what method is the test about. I usually middle click on it to bring the cursor to the type, or by looking at the test file name or the class name.

https://stackoverflow.com/questions/7321407/what-is-sut-and-where-did-it-come-from

Keeping the test object name consistent makes it easy to spot in any test classes. I have all my tests' SUT named testObj, and the reason for having all tests using the same test object name is because all the tests always have the same purpose of running the SUT method and check results. The business logic varies, but tests' purpose and flow stay the same.


@BeforeEach
void setUp() {
testObj = new AddressService(repository, mapper);
}

@Test
void findAllShouldCallRepositoryMethodAndMatchZip() {
Address address = new Address();
String zip = "test zip";
address.setZip(zip);
when(repository.findAll(any(Specification.class))).thenReturn(List.of(address));
List<AddressDto> results = testObj.findAllByUserId("");
assertEquals(zip, results.get(0).getZip());
verify(repository).findAll(any(Specification.class));
Copy link
Member

Choose a reason for hiding this comment

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

You should not verify with concepts like any() since the implementation can become flawed and your test won't catch that (because you're mocking the findAll. Instead you should only use any() in your when statements and use the actual objects in your verify statements. If you do this then you can be less specific in your when statements.

 final String userId = "";
 // mock data     

 when(repository.findAll(any()).thenReturn(List.of(address));

 final List<AddressDto> results = addressService.findAllByUserId(userId);
 // asserts 

 final AddressFilter filter = new AddressFilter(Set.of(userId));
 final AddressSpecification specification = new AddressSpecification(filter);
 verify(repository).findAll(specification);

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. Sometimes I tack it on the back of assertions just to make sure the method gets called but I guess it's not very useful in this case.

}
}