Skip to content

Commit

Permalink
Remove overzealous input validation
Browse files Browse the repository at this point in the history
  • Loading branch information
E0735389 committed Apr 2, 2024
1 parent 5be1b4b commit 044f213
Show file tree
Hide file tree
Showing 21 changed files with 194 additions and 146 deletions.
6 changes: 6 additions & 0 deletions src/main/java/seedu/address/logic/Messages.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package seedu.address.logic;

import static seedu.address.logic.parser.CliSyntax.PREFIX_FORCE;

import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand All @@ -16,6 +18,7 @@ public class Messages {
public static final String MESSAGE_UNCLEAR_COMMAND = "Unclear command";
public static final String MESSAGE_UNKNOWN_COMMAND = "Unknown command";
public static final String MESSAGE_INVALID_COMMAND_FORMAT = "Invalid command format! \n%1$s";
public static final String MESSAGE_FORCE_TAG_MUST_BE_EMPTY = "There must be no value following " + PREFIX_FORCE;
public static final String MESSAGE_UNEXPECTED_ARGUMENT = "Invalid command format!\n"
+ "Command '%1$s' should not take any arguments";
public static final String MESSAGE_INVALID_PERSON_DISPLAYED_INDEX = "The person index provided is invalid";
Expand All @@ -24,6 +27,9 @@ public class Messages {
public static final String MESSAGE_PERSONS_LISTED_OVERVIEW = "%1$d persons listed!";
public static final String MESSAGE_DUPLICATE_FIELDS =
"Multiple values specified for the following single-valued field(s): ";
public static final String MESSAGE_FIX_OR_ADD_FORCE_FLAG =
"The following is one of the invalid fields(s). "
+ "Fix the issue, or append " + PREFIX_FORCE + " to ignore the warning.\n%1$s";

/**
* Returns an error message indicating the duplicate prefixes.
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/seedu/address/logic/commands/AddCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static seedu.address.logic.parser.CliSyntax.PREFIX_ADDRESS;
import static seedu.address.logic.parser.CliSyntax.PREFIX_COURSE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_EMAIL;
import static seedu.address.logic.parser.CliSyntax.PREFIX_FORCE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_NAME;
import static seedu.address.logic.parser.CliSyntax.PREFIX_PHONE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_ROLE;
Expand Down Expand Up @@ -32,7 +33,8 @@ public class AddCommand extends Command {
+ PREFIX_COURSE + "COURSE "
+ "[" + PREFIX_ADDRESS + "ADDRESS] "
+ "[" + PREFIX_PHONE + "PHONE] "
+ "[" + PREFIX_TAG + "TAG]...\n"
+ "[" + PREFIX_TAG + "TAG]... "
+ "[" + PREFIX_FORCE + "]\n"
+ "Example: " + COMMAND_WORD + " "
+ PREFIX_NAME + "John Doe "
+ PREFIX_PHONE + "98765432 "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,24 @@ public class CommandExecutionException extends Exception implements CommandExcep
*/
private final Optional<CommandPart> erroneousPart;

private CommandExecutionException(String message, Optional<CommandPart> erroneousPart) {
/**
* Constructs a {@code CommandExecutionException} with the specified detail message and command part.
* @param message The detail message.
* @param erroneousPart The part of the command that causes the error, if any.
*/
public CommandExecutionException(String message, Optional<CommandPart> erroneousPart) {
super(message);
assert erroneousPart != null;
this.erroneousPart = erroneousPart;
}

private CommandExecutionException(String message, Throwable cause, Optional<CommandPart> erroneousPart) {
/**
* Constructs a {@code CommandExecutionException} with the specified detail message, command part, and cause.
* @param message The detail message.
* @param cause The cause of the error.
* @param erroneousPart The part of the command that causes the error, if any.
*/
public CommandExecutionException(String message, Throwable cause, Optional<CommandPart> erroneousPart) {
super(message, cause);
assert erroneousPart != null;
this.erroneousPart = erroneousPart;
Expand Down
30 changes: 21 additions & 9 deletions src/main/java/seedu/address/logic/parser/AddCommandParser.java
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package seedu.address.logic.parser;

import static seedu.address.logic.Messages.MESSAGE_FIX_OR_ADD_FORCE_FLAG;
import static seedu.address.logic.Messages.MESSAGE_INVALID_COMMAND_FORMAT;
import static seedu.address.logic.parser.CliSyntax.PREFIX_ADDRESS;
import static seedu.address.logic.parser.CliSyntax.PREFIX_COURSE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_EMAIL;
import static seedu.address.logic.parser.CliSyntax.PREFIX_FORCE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_NAME;
import static seedu.address.logic.parser.CliSyntax.PREFIX_PHONE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_ROLE;
Expand Down Expand Up @@ -39,24 +41,35 @@ public AddCommand parse(CommandPart args) throws ParseException {
ArgumentTokenizer.tokenize(
args,
PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ROLE,
PREFIX_ADDRESS, PREFIX_COURSE, PREFIX_TAG);
PREFIX_ADDRESS, PREFIX_COURSE, PREFIX_TAG, PREFIX_FORCE);

if (!arePrefixesPresent(argMultimap, PREFIX_NAME, PREFIX_EMAIL, PREFIX_ROLE, PREFIX_COURSE)
|| !argMultimap.getPreamble().isEmpty()) {
throw new ParseException(String.format(MESSAGE_INVALID_COMMAND_FORMAT, AddCommand.MESSAGE_USAGE));
}

argMultimap.verifyNoDuplicatePrefixesFor(PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS);
argMultimap.verifyNoDuplicatePrefixesFor(PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS, PREFIX_FORCE);

boolean shouldCheck = ParserUtil.parseShouldCheckFlag(argMultimap);

Name name = ParserUtil.parseName(argMultimap.getValue(PREFIX_NAME).get());
Optional<CommandPart> phoneString = argMultimap.getValue(PREFIX_PHONE);
Optional<Phone> phone;
if (phoneString.isPresent()) {
phone = Optional.of(ParserUtil.parsePhone(phoneString.get()));
} else {
phone = Optional.empty();
}
Email email;
Course course;
try {
if (phoneString.isPresent()) {
phone = Optional.of(ParserUtil.parsePhone(phoneString.get(), shouldCheck));
} else {
phone = Optional.empty();
}

Email email = ParserUtil.parseEmail(argMultimap.getValue(PREFIX_EMAIL).get());
email = ParserUtil.parseEmail(argMultimap.getValue(PREFIX_EMAIL).get(), shouldCheck);
course = ParserUtil.parseCourse(argMultimap.getValue(PREFIX_COURSE).get(), shouldCheck);
} catch (ParseException pe) {
throw new ParseException(String.format(MESSAGE_FIX_OR_ADD_FORCE_FLAG, pe.getMessage()),
pe.getErroneousPart());
}
Role role = ParserUtil.parseRole(argMultimap.getValue(PREFIX_ROLE).get());

Optional<CommandPart> addressString = argMultimap.getValue(PREFIX_ADDRESS);
Expand All @@ -76,7 +89,6 @@ public AddCommand parse(CommandPart args) throws ParseException {
}
}

Course course = ParserUtil.parseCourse(argMultimap.getValue(PREFIX_COURSE).get());
Set<Tag> tagList = ParserUtil.parseTags(argMultimap.getAllValues(PREFIX_TAG));

Person person = new Person(name, phone, email, role, address, course, tagList);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class AddressBookParser {
* Used for initial separation of command word and args.
*/
private static final Pattern BASIC_COMMAND_FORMAT =
Pattern.compile("\\s*(?<commandWord>\\S+)(?<arguments>.*?)\\s*");
Pattern.compile("\\s*(?<commandWord>\\w+)(?<arguments>.*?)\\s*");
private static final Logger logger = LogsCenter.getLogger(AddressBookParser.class);

/**
Expand Down
1 change: 1 addition & 0 deletions src/main/java/seedu/address/logic/parser/CliSyntax.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@ public class CliSyntax {
public static final Prefix PREFIX_ADDRESS = new Prefix("a/");
public static final Prefix PREFIX_COURSE = new Prefix("c/");
public static final Prefix PREFIX_TAG = new Prefix("t/");
public static final Prefix PREFIX_FORCE = new Prefix("f/");

}
32 changes: 22 additions & 10 deletions src/main/java/seedu/address/logic/parser/EditCommandParser.java
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package seedu.address.logic.parser;

import static java.util.Objects.requireNonNull;
import static seedu.address.logic.Messages.MESSAGE_FIX_OR_ADD_FORCE_FLAG;
import static seedu.address.logic.Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX_WITH_USAGE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_ADDRESS;
import static seedu.address.logic.parser.CliSyntax.PREFIX_COURSE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_EMAIL;
import static seedu.address.logic.parser.CliSyntax.PREFIX_FORCE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_NAME;
import static seedu.address.logic.parser.CliSyntax.PREFIX_PHONE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_ROLE;
Expand Down Expand Up @@ -36,7 +38,7 @@ public EditCommand parse(CommandPart args) throws ParseException {
ArgumentMultimap<CommandPart> argMultimap =
ArgumentTokenizer.tokenize(args,
PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL,
PREFIX_ROLE, PREFIX_ADDRESS, PREFIX_COURSE, PREFIX_TAG);
PREFIX_ROLE, PREFIX_ADDRESS, PREFIX_COURSE, PREFIX_TAG, PREFIX_FORCE);

Index index;

Expand All @@ -50,18 +52,31 @@ public EditCommand parse(CommandPart args) throws ParseException {

argMultimap.verifyNoDuplicatePrefixesFor(
PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL,
PREFIX_ROLE, PREFIX_ADDRESS, PREFIX_COURSE);
PREFIX_ROLE, PREFIX_ADDRESS, PREFIX_COURSE, PREFIX_FORCE);

boolean shouldCheck = ParserUtil.parseShouldCheckFlag(argMultimap);

EditPersonDescriptor editPersonDescriptor = new EditPersonDescriptor();

if (argMultimap.getValue(PREFIX_NAME).isPresent()) {
editPersonDescriptor.setName(ParserUtil.parseName(argMultimap.getValue(PREFIX_NAME).get()));
}
if (argMultimap.getValue(PREFIX_PHONE).isPresent()) {
editPersonDescriptor.setPhone(ParserUtil.parseOptionalPhone(argMultimap.getValue(PREFIX_PHONE).get()));
}
if (argMultimap.getValue(PREFIX_EMAIL).isPresent()) {
editPersonDescriptor.setEmail(ParserUtil.parseEmail(argMultimap.getValue(PREFIX_EMAIL).get()));
try {
if (argMultimap.getValue(PREFIX_PHONE).isPresent()) {
editPersonDescriptor.setPhone(
ParserUtil.parseOptionalPhone(argMultimap.getValue(PREFIX_PHONE).get(), shouldCheck));
}
if (argMultimap.getValue(PREFIX_EMAIL).isPresent()) {
editPersonDescriptor.setEmail(ParserUtil.parseEmail(argMultimap.getValue(PREFIX_EMAIL).get(),
shouldCheck));
}
if (argMultimap.getValue(PREFIX_COURSE).isPresent()) {
editPersonDescriptor.setCourse(ParserUtil.parseCourse(argMultimap.getValue(PREFIX_COURSE).get(),
shouldCheck));
}
} catch (ParseException pe) {
throw new ParseException(String.format(MESSAGE_FIX_OR_ADD_FORCE_FLAG, pe.getMessage()),
pe.getErroneousPart());
}
if (argMultimap.getValue(PREFIX_ROLE).isPresent()) {
editPersonDescriptor.setRole(ParserUtil.parseRole(argMultimap.getValue(PREFIX_ROLE).get()));
Expand All @@ -70,9 +85,6 @@ public EditCommand parse(CommandPart args) throws ParseException {
editPersonDescriptor.setAddress(
ParserUtil.parseOptionalAddress(argMultimap.getValue(PREFIX_ADDRESS).get()));
}
if (argMultimap.getValue(PREFIX_COURSE).isPresent()) {
editPersonDescriptor.setCourse(ParserUtil.parseCourse(argMultimap.getValue(PREFIX_COURSE).get()));
}
parseTagsForEdit(argMultimap.getAllValues(PREFIX_TAG)).ifPresent(editPersonDescriptor::setTags);

if (!editPersonDescriptor.isAnyFieldEdited()) {
Expand Down
65 changes: 50 additions & 15 deletions src/main/java/seedu/address/logic/parser/ParserUtil.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package seedu.address.logic.parser;

import static java.util.Objects.requireNonNull;
import static seedu.address.logic.Messages.MESSAGE_FORCE_TAG_MUST_BE_EMPTY;
import static seedu.address.logic.parser.CliSyntax.PREFIX_FORCE;

import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -86,29 +88,41 @@ public static Name parseName(CommandPart name) throws ParseException {
* Parses a {@code CommandPart phone} into a {@code Phone}.
* Leading and trailing whitespaces will be trimmed.
*
* @param shouldCheck If true, check if the phone number is valid.
* @throws ParseException if the given {@code phone} is invalid.
*/
public static Phone parsePhone(CommandPart phone) throws ParseException {
public static Phone parsePhone(CommandPart phone, boolean shouldCheck) throws ParseException {
requireNonNull(phone);
CommandPart trimmedPhone = phone.trim();
if (!Phone.isValidPhone(trimmedPhone.toString())) {
throw new ParseException(Phone.MESSAGE_CONSTRAINTS, trimmedPhone);
try {
return new Phone(trimmedPhone.toString(), shouldCheck);
} catch (IllegalArgumentException e) {
throw new ParseException(e.getMessage(), trimmedPhone);
}
return new Phone(trimmedPhone.toString());
}

public static Phone parsePhone(CommandPart phone) throws ParseException {
return parsePhone(phone, true);
}

/**
* Parses a {@code CommandPart phone} into a {@code Optional<Phone>}, allowing empty input.
* Leading and trailing whitespaces will be trimmed.
*
* @param shouldCheck If true, check if the phone number is valid.
* @throws ParseException if the given {@code phone} is invalid.
*/
public static Optional<Phone> parseOptionalPhone(CommandPart phone) throws ParseException {
public static Optional<Phone> parseOptionalPhone(CommandPart phone, boolean shouldCheck) throws ParseException {
requireNonNull(phone);
if (phone.trim().isEmpty()) {
CommandPart trimmedPhone = phone.trim();
if (trimmedPhone.isEmpty()) {
return Optional.empty();
}
return Optional.of(parsePhone(phone.trim()));
return Optional.of(parsePhone(trimmedPhone, shouldCheck));
}

public static Optional<Phone> parseOptionalPhone(CommandPart phone) throws ParseException {
return parseOptionalPhone(phone, true);
}

/**
Expand Down Expand Up @@ -145,30 +159,38 @@ public static Optional<Address> parseOptionalAddress(CommandPart address) throws
* Parses a {@code CommandPart email} into an {@code Email}.
* Leading and trailing whitespaces will be trimmed.
*
* @param shouldCheck If true, check if the phone number is valid.
* @throws ParseException if the given {@code email} is invalid.
*/
public static Email parseEmail(CommandPart email) throws ParseException {
public static Email parseEmail(CommandPart email, boolean shouldCheck) throws ParseException {
requireNonNull(email);
CommandPart trimmedEmail = email.trim();
if (!Email.isValidEmail(trimmedEmail.toString())) {
throw new ParseException(Email.MESSAGE_CONSTRAINTS, trimmedEmail);
try {
return new Email(trimmedEmail.toString(), shouldCheck);
} catch (IllegalArgumentException e) {
throw new ParseException(e.getMessage(), trimmedEmail);
}
return new Email(trimmedEmail.toString());
}

public static Email parseEmail(CommandPart email) throws ParseException {
return parseEmail(email, true);
}

/**
* Parses a {@code CommandPart course} into an {@code Course}.
* Leading and trailing whitespaces will be trimmed.
*
* @param shouldCheck If true, check if the course is valid.
* @throws ParseException if the given {@code course} is invalid.
*/
public static Course parseCourse(CommandPart course) throws ParseException {
public static Course parseCourse(CommandPart course, boolean shouldCheck) throws ParseException {
requireNonNull(course);
CommandPart trimmedCourse = course.trim();
if (!Course.isValidCourse(trimmedCourse.toString())) {
throw new ParseException(Course.MESSAGE_CONSTRAINTS, trimmedCourse);
try {
return new Course(trimmedCourse.toString(), shouldCheck);
} catch (IllegalArgumentException e) {
throw new ParseException(e.getMessage(), trimmedCourse);
}
return new Course(trimmedCourse.toString());
}

/**
Expand Down Expand Up @@ -237,4 +259,17 @@ public static List<String> filterByPrefix(String prefix, String[] strings) {

return matchedStrings;
}

/**
* Parses the {@code shouldCheck} flag from the argument multimap.
* @return true if the {@code PREFIX_FORCE} flag is absent, false otherwise.
*/
public static boolean parseShouldCheckFlag(ArgumentMultimap<CommandPart> argMultimap) throws ParseException {
boolean isForced = argMultimap.getValue(PREFIX_FORCE).isPresent();
if (isForced && !argMultimap.getValue(PREFIX_FORCE).get().isEmpty()) {
throw new ParseException(MESSAGE_FORCE_TAG_MUST_BE_EMPTY, argMultimap.getValue(PREFIX_FORCE).get());
}
boolean shouldCheck = !isForced;
return shouldCheck;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,24 @@ public class ParseException extends IllegalValueException implements CommandExce
*/
private final Optional<CommandPart> erroneousPart;

private ParseException(String message, Optional<CommandPart> erroneousPart) {
/**
* Constructs a {@code ParseException} with the specified detail message and command part.
* @param message The detail message.
* @param erroneousPart The part of the command that causes the error, if any.
*/
public ParseException(String message, Optional<CommandPart> erroneousPart) {
super(message);
assert erroneousPart != null;
this.erroneousPart = erroneousPart;
}

private ParseException(String message, Throwable cause, Optional<CommandPart> erroneousPart) {
/**
* Constructs a {@code ParseException} with the specified detail message, command part, and cause.
* @param message The detail message.
* @param cause The cause of the error.
* @param erroneousPart The part of the command that causes the error, if any.
*/
public ParseException(String message, Throwable cause, Optional<CommandPart> erroneousPart) {
super(message, cause);
assert erroneousPart != null;
this.erroneousPart = erroneousPart;
Expand Down
16 changes: 14 additions & 2 deletions src/main/java/seedu/address/model/person/Course.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,25 @@ public class Course {
* Constructs a {@code Course}.
*
* @param course A valid course code.
* @param shouldCheck If true, check if the course code is valid.
*/
public Course(String course) {
public Course(String course, boolean shouldCheck) {
requireNonNull(course);
checkArgument(isValidCourse(course), MESSAGE_CONSTRAINTS);
if (shouldCheck) {
checkArgument(isValidCourse(course), MESSAGE_CONSTRAINTS);
}
this.value = course.toUpperCase().trim();
}

/**
* Constructs a valid {@code Course}.
*
* @param course A valid course code.
*/
public Course(String course) {
this(course, true);
}

/**
* Returns true if a given string is a valid course code.
*/
Expand Down
Loading

0 comments on commit 044f213

Please sign in to comment.