Skip to content

Commit

Permalink
util, jaxrs: Add (enforce) ability to specify applicableObjectTypes w…
Browse files Browse the repository at this point in the history 
…hen creating ControlTagDefinition. Fixes #759
  • Loading branch information
@sbrossie
sbrossie committed Oct 12, 2017
1 parent f2d0c32 commit 8719ab4
Show file tree
Hide file tree
Showing 23 changed files with 147 additions and 59 deletions.
2 changes: 1 addition & 1 deletion account/src/test/java/org/killbill/billing/account/dao/TestAccountDao.java
View file
Expand Up @@ -184,7 +184,7 @@ public void testCustomFields() throws CustomFieldApiException {
@Test(groups = "slow", description = "Test Account DAO: tags")
public void testTags() throws TagApiException, TagDefinitionApiException {
final AccountModelDao account = createTestAccount();
final TagDefinitionModelDao tagDefinition = tagDefinitionDao.create(UUID.randomUUID().toString().substring(0, 4), UUID.randomUUID().toString(), internalCallContext);
final TagDefinitionModelDao tagDefinition = tagDefinitionDao.create(UUID.randomUUID().toString().substring(0, 4), UUID.randomUUID().toString(), ObjectType.ACCOUNT.name(), internalCallContext);
final Tag tag = new DescriptiveTag(tagDefinition.getId(), ObjectType.ACCOUNT, account.getId(), internalCallContext.getCreatedDate());
tagDao.create(new TagModelDao(tag), internalCallContext);

Expand Down
10 changes: 9 additions & 1 deletion beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestTagApi.java
View file
Expand Up @@ -35,6 +35,9 @@
import org.killbill.billing.util.tag.Tag;
import org.killbill.billing.util.tag.TagDefinition;

import com.google.common.collect.ImmutableSet;

import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;

public class TestTagApi extends TestIntegrationBase {
Expand Down Expand Up @@ -95,9 +98,14 @@ public void testApiTagOnInvoice() throws Exception {
// Create a new tag definition
//
busHandler.pushExpectedEvents(NextEvent.TAG_DEFINITION);
final TagDefinition tagDefinition = tagUserApi.createTagDefinition("foo", "foo desc", callContext);
final TagDefinition tagDefinition = tagUserApi.createTagDefinition("foo", "foo desc", ImmutableSet.<ObjectType>of(ObjectType.ACCOUNT, ObjectType.INVOICE), callContext);
assertListenerStatus();

final TagDefinition tagDefinition2 = tagUserApi.getTagDefinition(tagDefinition.getId(), callContext);
assertEquals(tagDefinition2.getApplicableObjectTypes().size(), 2);
assertEquals(tagDefinition2.getApplicableObjectTypes().get(0), ObjectType.ACCOUNT);
assertEquals(tagDefinition2.getApplicableObjectTypes().get(1), ObjectType.INVOICE);

//
// Add 2 Tags on the invoice (1 invoice tag and 1 user tag)
//
Expand Down
19 changes: 15 additions & 4 deletions jaxrs/src/main/java/org/killbill/billing/jaxrs/json/TagDefinitionJson.java
View file
Expand Up @@ -17,6 +17,7 @@
package org.killbill.billing.jaxrs.json;

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

import javax.annotation.Nullable;

Expand All @@ -29,6 +30,7 @@
import com.google.common.base.Function;
import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import io.swagger.annotations.ApiModelProperty;

public class TagDefinitionJson extends JsonBase {
Expand All @@ -40,14 +42,14 @@ public class TagDefinitionJson extends JsonBase {
private final String name;
@ApiModelProperty(required = true)
private final String description;
private final List<String> applicableObjectTypes;
private final Set<String> applicableObjectTypes;

@JsonCreator
public TagDefinitionJson(@JsonProperty("id") final String id,
@JsonProperty("isControlTag") final Boolean isControlTag,
@JsonProperty("name") final String name,
@JsonProperty("description") @Nullable final String description,
@JsonProperty("applicableObjectTypes") @Nullable final List<String> applicableObjectTypes,
@JsonProperty("applicableObjectTypes") @Nullable final Set<String> applicableObjectTypes,
@JsonProperty("auditLogs") @Nullable final List<AuditLogJson> auditLogs) {
super(auditLogs);
this.id = id;
Expand All @@ -62,7 +64,7 @@ public TagDefinitionJson(final TagDefinition tagDefinition, @Nullable final List
tagDefinition.isControlTag(),
tagDefinition.getName(),
tagDefinition.getDescription(),
ImmutableList.<String>copyOf(Collections2.transform(tagDefinition.getApplicableObjectTypes(), new Function<ObjectType, String>() {
ImmutableSet.<String>copyOf(Collections2.transform(tagDefinition.getApplicableObjectTypes(), new Function<ObjectType, String>() {
@Override
public String apply(@Nullable final ObjectType input) {
if (input == null) {
Expand Down Expand Up @@ -92,7 +94,7 @@ public String getDescription() {
return description;
}

public List<String> getApplicableObjectTypes() {
public Set<String> getApplicableObjectTypes() {
return applicableObjectTypes;
}

Expand Down Expand Up @@ -156,4 +158,13 @@ public int hashCode() {
result = 31 * result + (applicableObjectTypes != null ? applicableObjectTypes.hashCode() : 0);
return result;
}

public static Set<ObjectType> toObjectType(final Set<String> applicableObjectTypes) {
return ImmutableSet.copyOf(Collections2.transform(applicableObjectTypes, new Function<String, ObjectType>() {
@Override
public ObjectType apply(final String input) {
return ObjectType.valueOf(input);
}
}));
}
}
6 changes: 5 additions & 1 deletion jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/TagDefinitionResource.java
View file
Expand Up @@ -52,6 +52,7 @@
import org.killbill.clock.Clock;
import org.killbill.commons.metrics.TimedResource;

import com.google.common.base.Preconditions;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import io.swagger.annotations.Api;
Expand Down Expand Up @@ -129,8 +130,11 @@ public Response createTagDefinition(final TagDefinitionJson json,
verifyNonNullOrEmpty(json, "TagDefinitionJson body should be specified");
verifyNonNullOrEmpty(json.getName(), "TagDefinition name needs to be set",
json.getDescription(), "TagDefinition description needs to be set");
Preconditions.checkArgument(json.getApplicableObjectTypes() != null &&

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Oct 23, 2017

Member

Alternatively, we could default to all (current behavior)?

This comment has been minimized.

Sign in to view

Copy link
@sbrossie

sbrossie Oct 23, 2017

Author Member

We could, but i have never use cases where Tags apply to all objects, so would that really be a good default?

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Oct 23, 2017

Member

Fair enough - I was just thinking about backward compatibility but it probably doesn't matter.
!json.getApplicableObjectTypes().isEmpty(), "Applicable object types must be set");

final TagDefinition createdTagDef = tagUserApi.createTagDefinition(json.getName(), json.getDescription(), context.createCallContextNoAccountId(createdBy, reason, comment, request));

final TagDefinition createdTagDef = tagUserApi.createTagDefinition(json.getName(), json.getDescription(), TagDefinitionJson.toObjectType(json.getApplicableObjectTypes()), context.createCallContextNoAccountId(createdBy, reason, comment, request));
return uriBuilder.buildResponse(uriInfo, TagDefinitionResource.class, "getTagDefinition", createdTagDef.getId(), request);
}

Expand Down
3 changes: 2 additions & 1 deletion jaxrs/src/test/java/org/killbill/billing/jaxrs/json/TestTagDefinitionJson.java
View file
Expand Up @@ -24,6 +24,7 @@
import org.killbill.billing.jaxrs.JaxrsTestSuiteNoDB;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;

public class TestTagDefinitionJson extends JaxrsTestSuiteNoDB {

Expand All @@ -33,7 +34,7 @@ public void testJson() throws Exception {
final Boolean isControlTag = true;
final String name = UUID.randomUUID().toString();
final String description = UUID.randomUUID().toString();
final ImmutableList<String> applicableObjectTypes = ImmutableList.<String>of(UUID.randomUUID().toString());
final ImmutableSet<String> applicableObjectTypes = ImmutableSet.<String>of(UUID.randomUUID().toString());
final TagDefinitionJson tagDefinitionJson = new TagDefinitionJson(id, isControlTag, name, description, applicableObjectTypes, null);
Assert.assertEquals(tagDefinitionJson.getId(), id);
Assert.assertEquals(tagDefinitionJson.isControlTag(), isControlTag);
Expand Down
5 changes: 3 additions & 2 deletions profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestPayment.java
View file
Expand Up @@ -26,6 +26,7 @@

import javax.annotation.Nullable;

import org.killbill.billing.ObjectType;
import org.killbill.billing.client.KillBillClientException;
import org.killbill.billing.client.RequestOptions;
import org.killbill.billing.client.model.Account;
Expand Down Expand Up @@ -696,7 +697,7 @@ public void testComboAuthorizationInvalidPaymentMethod() throws Exception {
public void testGetTagsForPaymentTransaction() throws Exception {
UUID tagDefinitionId = UUID.randomUUID();
String tagDefinitionName = "payment-transaction";
TagDefinition tagDefinition = new TagDefinition(tagDefinitionId, false, tagDefinitionName, "description", null);
TagDefinition tagDefinition = new TagDefinition(tagDefinitionId, false, tagDefinitionName, "description", ImmutableList.<ObjectType>of(ObjectType.TRANSACTION));
final TagDefinition createdTagDefinition = killBillClient.createTagDefinition(tagDefinition, requestOptions);

final Account account = createAccountWithDefaultPaymentMethod();
Expand All @@ -719,7 +720,7 @@ public void testGetTagsForPaymentTransaction() throws Exception {
public void testCreateTagForPaymentTransaction() throws Exception {
UUID tagDefinitionId = UUID.randomUUID();
String tagDefinitionName = "payment-transaction";
TagDefinition tagDefinition = new TagDefinition(tagDefinitionId, false, tagDefinitionName, "description", null);
TagDefinition tagDefinition = new TagDefinition(tagDefinitionId, false, tagDefinitionName, "description", ImmutableList.<ObjectType>of(ObjectType.TRANSACTION));
final TagDefinition createdTagDefinition = killBillClient.createTagDefinition(tagDefinition, requestOptions);

final Account account = createAccountWithDefaultPaymentMethod();
Expand Down
18 changes: 9 additions & 9 deletions profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestTag.java
View file
Expand Up @@ -49,9 +49,9 @@ public class TestTag extends TestJaxrsBase {

@Test(groups = "slow", description = "Cannot add badly formatted TagDefinition")
public void testTagErrorHandling() throws Exception {
final TagDefinition[] tagDefinitions = {new TagDefinition(null, false, null, null, null),
new TagDefinition(null, false, "something", null, null),
new TagDefinition(null, false, null, "something", null)};
final TagDefinition[] tagDefinitions = {new TagDefinition(null, false, null, null, ImmutableList.<ObjectType>of(ObjectType.ACCOUNT)),
new TagDefinition(null, false, "something", null, ImmutableList.<ObjectType>of(ObjectType.INVOICE)),
new TagDefinition(null, false, null, "something", ImmutableList.<ObjectType>of(ObjectType.TRANSACTION))};

for (final TagDefinition tagDefinition : tagDefinitions) {
try {
Expand All @@ -66,7 +66,7 @@ public void testTagErrorHandling() throws Exception {

@Test(groups = "slow", description = "Can create a TagDefinition")
public void testTagDefinitionOk() throws Exception {
final TagDefinition input = new TagDefinition(null, false, "blue", "relaxing color", ImmutableList.<ObjectType>of());
final TagDefinition input = new TagDefinition(null, false, "blue", "relaxing color", ImmutableList.<ObjectType>of(ObjectType.TRANSACTION));

final TagDefinition objFromJson = killBillClient.createTagDefinition(input, requestOptions);
assertNotNull(objFromJson);
Expand All @@ -80,16 +80,16 @@ public void testMultipleTagDefinitionOk() throws Exception {
final int sizeSystemTag = objFromJson.isEmpty() ? 0 : objFromJson.size();


final TagDefinition inputBlue = new TagDefinition(null, false, "blue", "relaxing color", ImmutableList.<ObjectType>of());
final TagDefinition inputBlue = new TagDefinition(null, false, "blue", "relaxing color", ImmutableList.<ObjectType>of(ObjectType.TRANSACTION));
killBillClient.createTagDefinition(inputBlue, requestOptions);

final TagDefinition inputRed = new TagDefinition(null, false, "red", "hot color", ImmutableList.<ObjectType>of());
final TagDefinition inputRed = new TagDefinition(null, false, "red", "hot color", ImmutableList.<ObjectType>of(ObjectType.TRANSACTION));
killBillClient.createTagDefinition(inputRed, requestOptions);

final TagDefinition inputYellow = new TagDefinition(null, false, "yellow", "vibrant color", ImmutableList.<ObjectType>of());
final TagDefinition inputYellow = new TagDefinition(null, false, "yellow", "vibrant color", ImmutableList.<ObjectType>of(ObjectType.TRANSACTION));
killBillClient.createTagDefinition(inputYellow, requestOptions);

final TagDefinition inputGreen = new TagDefinition(null, false, "green", "super relaxing color", ImmutableList.<ObjectType>of());
final TagDefinition inputGreen = new TagDefinition(null, false, "green", "super relaxing color", ImmutableList.<ObjectType>of(ObjectType.TRANSACTION));
killBillClient.createTagDefinition(inputGreen, requestOptions);

objFromJson = killBillClient.getTagDefinitions(requestOptions);
Expand Down Expand Up @@ -119,7 +119,7 @@ public void testGetAllTagsByType() throws Exception {
killBillClient.createAccountTag(account.getAccountId(), controlTagType.getId(), requestOptions);
}

final TagDefinition bundleTagDefInput = new TagDefinition(null, false, "bundletagdef", "nothing special", ImmutableList.<ObjectType>of());
final TagDefinition bundleTagDefInput = new TagDefinition(null, false, "bundletagdef", "nothing special", ImmutableList.<ObjectType>of(ObjectType.TRANSACTION));
final TagDefinition bundleTagDef = killBillClient.createTagDefinition(bundleTagDefInput, requestOptions);

killBillClient.createBundleTag(subscriptionJson.getBundleId(), bundleTagDef.getId(), requestOptions);
Expand Down
33 changes: 27 additions & 6 deletions util/src/main/java/org/killbill/billing/util/tag/DefaultTagDefinition.java
View file
Expand Up @@ -19,39 +19,50 @@
import java.util.List;
import java.util.UUID;

import javax.annotation.Nullable;

import org.killbill.billing.ObjectType;
import org.killbill.billing.entity.EntityBase;
import org.killbill.billing.util.UUIDs;
import org.killbill.billing.util.tag.dao.TagDefinitionModelDao;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Function;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;

public class DefaultTagDefinition extends EntityBase implements TagDefinition {

private static final Splitter SPLITTER = Splitter.on(',').omitEmptyStrings().trimResults();

private final String name;
private final String description;
private final Boolean controlTag;
private final List<ObjectType> applicableObjectTypes;


public DefaultTagDefinition(final TagDefinitionModelDao tagDefinitionModelDao, final boolean isControlTag) {
this(tagDefinitionModelDao.getId(), tagDefinitionModelDao.getName(), tagDefinitionModelDao.getDescription(), isControlTag);
this(tagDefinitionModelDao.getId(), tagDefinitionModelDao.getName(), tagDefinitionModelDao.getDescription(), isControlTag, toObjectTypes(tagDefinitionModelDao.getApplicableObjectTypes()));
}


public DefaultTagDefinition(final ControlTagType controlTag) {
this(controlTag.getId(), controlTag.toString(), controlTag.getDescription(), true, controlTag.getApplicableObjectTypes());
}

// Test only
public DefaultTagDefinition(final String name, final String description, final Boolean isControlTag) {
this(UUIDs.randomUUID(), name, description, isControlTag);
}

// Test only
public DefaultTagDefinition(final UUID id, final String name, final String description, final Boolean isControlTag) {
this(id, name, description, isControlTag, getApplicableObjectTypes(id, isControlTag));
}

public DefaultTagDefinition(final ControlTagType controlTag) {
this(controlTag.getId(), controlTag.toString(), controlTag.getDescription(), true, controlTag.getApplicableObjectTypes());
}


@JsonCreator
public DefaultTagDefinition(@JsonProperty("id") final UUID id,
@JsonProperty("name") final String name,
Expand Down Expand Up @@ -144,4 +155,14 @@ private static List<ObjectType> getApplicableObjectTypes(final UUID id, final Bo
}
throw new IllegalStateException(String.format("ControlTag id %s does not seem to exist", id));
}

private static List<ObjectType> toObjectTypes(final String input) {
return ImmutableList.copyOf(Iterables.transform(SPLITTER.splitToList(input), new Function<String, ObjectType>() {
@Override
public ObjectType apply(final String input) {
return ObjectType.valueOf(input);
}
}));
}

}
8 changes: 6 additions & 2 deletions util/src/main/java/org/killbill/billing/util/tag/api/DefaultTagUserApi.java
View file
Expand Up @@ -18,6 +18,7 @@

import java.util.Collection;
import java.util.List;
import java.util.Set;
import java.util.UUID;

import org.killbill.billing.ErrorCode;
Expand Down Expand Up @@ -45,6 +46,7 @@
import org.killbill.billing.util.tag.dao.TagModelDaoHelper;

import com.google.common.base.Function;
import com.google.common.base.Joiner;
import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
import com.google.inject.Inject;
Expand All @@ -53,6 +55,8 @@

public class DefaultTagUserApi implements TagUserApi {

private static final Joiner JOINER = Joiner.on(",");

private static final Function<TagModelDao, Tag> TAG_MODEL_DAO_TAG_FUNCTION = new Function<TagModelDao, Tag>() {
@Override
public Tag apply(final TagModelDao input) {
Expand Down Expand Up @@ -85,11 +89,11 @@ public TagDefinition apply(final TagDefinitionModelDao input) {
}

@Override
public TagDefinition createTagDefinition(final String definitionName, final String description, final CallContext context) throws TagDefinitionApiException {
public TagDefinition createTagDefinition(final String definitionName, final String description, final Set<ObjectType> applicableObjectTypes, final CallContext context) throws TagDefinitionApiException {
if (definitionName.matches(".*[A-Z].*")) {
throw new TagDefinitionApiException(ErrorCode.TAG_DEFINITION_HAS_UPPERCASE, definitionName);
}
final TagDefinitionModelDao tagDefinitionModelDao = tagDefinitionDao.create(definitionName, description, internalCallContextFactory.createInternalCallContextWithoutAccountRecordId(context));
final TagDefinitionModelDao tagDefinitionModelDao = tagDefinitionDao.create(definitionName, description, JOINER.join(applicableObjectTypes), internalCallContextFactory.createInternalCallContextWithoutAccountRecordId(context));
return new DefaultTagDefinition(tagDefinitionModelDao, TagModelDaoHelper.isControlTag(tagDefinitionModelDao.getName()));
}

Expand Down
6 changes: 4 additions & 2 deletions util/src/main/java/org/killbill/billing/util/tag/dao/DefaultTagDefinitionDao.java
View file
Expand Up @@ -22,6 +22,7 @@
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.Set;
import java.util.UUID;

import org.killbill.billing.BillingExceptionBase;
Expand Down Expand Up @@ -59,6 +60,7 @@ public class DefaultTagDefinitionDao extends EntityDaoBase<TagDefinitionModelDao
private final TagEventBuilder tagEventBuilder;
private final PersistentBus bus;


@Inject
public DefaultTagDefinitionDao(final IDBI dbi, final TagEventBuilder tagEventBuilder, final PersistentBus bus, final Clock clock,
final CacheControllerDispatcher controllerDispatcher, final NonEntityDao nonEntityDao, final InternalCallContextFactory internalCallContextFactory) {
Expand Down Expand Up @@ -134,7 +136,7 @@ public String apply(final UUID input) {
}

@Override
public TagDefinitionModelDao create(final String definitionName, final String description,
public TagDefinitionModelDao create(final String definitionName, final String description, final String tagDefinitionObjectTypes,
final InternalCallContext context) throws TagDefinitionApiException {
// Make sure a invoice tag with this name don't already exist
if (TagModelDaoHelper.isControlTag(definitionName)) {
Expand All @@ -154,7 +156,7 @@ public TagDefinitionModelDao inTransaction(final EntitySqlDaoWrapperFactory enti
}

// Create it
final TagDefinitionModelDao tagDefinition = new TagDefinitionModelDao(context.getCreatedDate(), definitionName, description);
final TagDefinitionModelDao tagDefinition = new TagDefinitionModelDao(context.getCreatedDate(), definitionName, description, tagDefinitionObjectTypes);
createAndRefresh(tagDefinitionSqlDao, tagDefinition, context);

// Post an event to the bus
Expand Down

2 comments on commit 8719ab4

@pierre
Copy link
Member

@pierre pierre commented on 8719ab4 Oct 23, 2017

Choose a reason for hiding this comment

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

Should we also add validation that the object tagged is of the right applicable type? It should already be done implicitly in JAX-RS, but not at the Java API level.

@sbrossie
Copy link
Member Author

Choose a reason for hiding this comment

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

Will add the test assertion.

Please sign in to comment.