Error in this repeat loop. It is only taking the first value of the database

Question

Error in this loop of repetition. It is only taking the first value of the database. You’re just doing the consultation in the first field and you’re ignoring the others.

@Controller
// @RequestMapping("/login")
public class LoginController extends HttpServlet {


    private static final long serialVersionUID = 1L;
    private String username;
    private String password;

    @Autowired
    Usuarios usuarios;


    HttpServletRequest request;
    HttpServletResponse response;

    public String getUsername() {
        return username;
    }

    public void setUsername(String username) {
        this.username = username;
    }

    public String getPassword() {
        return password;
    }

    public void setPassword(String password) {
        this.password = password;
    }

    @RequestMapping("/login")
    public ModelAndView logins() {
        ModelAndView mv = new ModelAndView("/login");
        mv.addObject(new Documento());
        return mv;
    }

    @RequestMapping("/efetuaLogin")
    public ModelAndView login(HttpServletRequest request, HttpServletResponse response) {

        boolean validacao = false;

        HttpSession sessao;

        List<Usuario> usuariosCadastrados = usuarios.lista();


        String username = request.getParameter("username");
        String password = request.getParameter("password");

        for (Usuario usuario : usuariosCadastrados) {

            String loginbd = usuario.getUsername();
            String senhabd = usuario.getPassword();

            System.out.println("username do Formulario...:" + loginbd);
            System.out.println("Senha do banco........:" + senhabd);
            System.out.println("Senha do Formulario...:" + password);
            System.out.println("username do Formulario...:" + username);

            if (username.equals(loginbd) && password.equals(senhabd)) {
                validacao = true;

            }
            if (validacao == true) {

                return new ModelAndView("/documentos");

            } else {
                return new ModelAndView("hello");

            }
        }
        return null;
    }
}

    @Service
public class LoginService {

    @PersistenceContext
      private EntityManager em;

    @Autowired
    private Usuarios usuarios; 

    public List<Usuario> lista() {
      return em.createQuery("select u from Usuario u", Usuario.class).getResultList();
    }
}

Answer 1

The reason to go wrong is the structure of your for. Let’s take a look:

    for (Usuario usuario : usuariosCadastrados) {

        // Várias linhas de código...

        if (validacao == true) {
            return new ModelAndView("/documentos");
        } else {
            return new ModelAndView("hello");
        }
    }
    return null;

Note that in the first iteration, it will either fall or if or in the else, and in both cases, a return shall be executed. However, if a return will always run in the first iteration, so it will always stop everything right in the first interaction.

What you wanted was to keep looking instead of giving one return in the else:

    for (Usuario usuario : usuariosCadastrados) {

        String loginbd = usuario.getUsername();
        String senhabd = usuario.getPassword();

        System.out.println("username do Formulario...:" + loginbd);
        System.out.println("Senha do banco........:" + senhabd);
        System.out.println("Senha do Formulario...:" + password);
        System.out.println("username do Formulario...:" + username);

        if (username.equals(loginbd) && password.equals(senhabd)) {
            validacao = true;

        }
        if (validacao == true) {
            return new ModelAndView("/documentos");
        }
    }
    return new ModelAndView("hello");

However, there are still a lot more improvements that can be made. Use == true for example is totally unnecessary, and the variable becomes unnecessary since it serves only to force the flow to reach the return right after. The variable sessao also not being used. With that in mind, let’s simplify your code even more:

@RequestMapping("/efetuaLogin")
public ModelAndView login(HttpServletRequest request, HttpServletResponse response) {
    List<Usuario> usuariosCadastrados = usuarios.lista();
    String username = request.getParameter("username");
    String password = request.getParameter("password");

    for (Usuario usuario : usuariosCadastrados) {
        String loginbd = usuario.getUsername();
        String senhabd = usuario.getPassword();
        System.out.println("username do Formulario...:" + loginbd);
        System.out.println("Senha do banco........:" + senhabd);
        System.out.println("Senha do Formulario...:" + password);
        System.out.println("username do Formulario...:" + username);

        if (username.equals(loginbd) && password.equals(senhabd)) {
            return new ModelAndView("/documentos");
        }
    }
    return new ModelAndView("hello");
}

However, you’re taking every user in the database and looking one by one to look for the correct login and password, which is inefficient. It would be much better to search the database only the user you want. The database is the ideal place to perform this type of search, since it has data indexing algorithms quite optimized for searches and by doing so you also significantly reduce the volume of data traffic between the database and the application, which also improves performance and memory cosumo. Therefore, your LoginService gets like this:

@Service
public class LoginService {

    @PersistenceContext
    private EntityManager em;

    @Autowired
    private Usuarios usuarios; 

    public Usuario logar(String login, String senha) {
        try {
            return em.createQuery("SELECT u FROM Usuario u WHERE u.username = :login AND u.password = :senha", Usuario.class)
                    .setParameter("login", login)
                    .setParameter("senha", senha)
                    .getSingleResult();
        } catch (NoResultException e) {
            return null;
        }
    }
}

Your login method looks like this:

@RequestMapping("/efetuaLogin")
public ModelAndView login(HttpServletRequest request, HttpServletResponse response) {
    List<Usuario> usuariosCadastrados = usuarios.lista();
    String username = request.getParameter("username");
    String password = request.getParameter("password");
    Usuario usuario = usuarios.logar(username, password)';
    if (usuario != null) return new ModelAndView("/documentos");
    return new ModelAndView("hello");
}

Also, you are bringing the data from the database, through the class Usuarios and not through the class LoginService. However, you did not enter the class code Usuarios in your question, then you can’t tell whether everything is right there or not. However, since you are now using a new method to search the database, the class Usuarios will also have to be amended.

And finally, let’s see the rest of your controller:

She inherits from HttpServlet, which suggests that a single instance of this class will be created and shared by all requests. However, you have put the methods setPassword, getPassword, setUsername, getUsername and the fields username, password, request and response, all of them are specific to each particular request. The result of this will be bad, as you are putting in a place shared among all requests, data that belongs to each request individually, and so if two requests use these methods and fields simultaneously the result will be disastrous.

To solve this problem, simply delete such methods and fields, as they are unnecessary since the data relating to them you may need are already in your method login.