Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,6 @@

public interface ODPApiManager {
String fetchQualifiedSegments(String apiKey, String apiEndpoint, String userKey, String userValue, List<String> segmentsToCheck);

void sendEvents(String apiKey, String apiEndpoint, List<ODPEvent> events);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you plan to open these to public (and Android-SDK) to customize. If so, ODPEvent should be open too, but this is not desired. Let's discuss more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any methods that make any external http calls have to be passed in from outside so that we can provide separate implementations for java and android. For java sdk only, the core part is in the core-api and the http implementation has to be in core-httpclient-impl. For Android, we will re use all the ODP code int he core-api but write a new ODPApiManager implementation and pass from outside. This is why it's open. Let me know what you think?

Copy link
Contributor

@jaeopt jaeopt Aug 25, 2022

Choose a reason for hiding this comment

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

@zashraf1985 I fully understand opening is required to support Android and other clients. My question was about opening "ODPEvent" type. We can consider to accept serialized String or map instead here, that's how we avoid internal type.

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 would emphasize on keeping the ODPEvent type exposed here. It's only and Entity class which does not contain any business logic. Adding a string input here will increase the burden of unnecessary serialization on us. Keeping it a list of ODPEvent is easy to use and less error prone. What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

@zashraf1985 I understand that it'll look nice to pass the structure, but opening ODPEvent may be expensive. Following our rule, we should rename it to "OptimizelyOdpEvent" like other public type, which also should be documented. Other SDKs also should be changed for consistency. A question is that it's worth to open it for the cost? Clients do not care about the contents, just serialized and dispatched.

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 think you are right. The only responsibility of the httpclient should be to take the payload, send it and return status. The serialization should be handled in EventManager. I will change it to get the string payload. The serialization code is still useful for the ODPEventManager so i will leave it there with this PR.

64 changes: 64 additions & 0 deletions core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* Copyright 2022, Optimizely Inc. and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.odp;

import java.util.Map;

public class ODPEvent {
private String type;
private String action;
private Map<String, String > identifiers;
private Map<String, String> data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private Map<String, String> data;
private Map<String, Object> data;

It can support any type of data (String, Number, Boolean).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!


public ODPEvent(String type, String action, Map<String, String> identifiers, Map<String, String> data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public ODPEvent(String type, String action, Map<String, String> identifiers, Map<String, String> data) {
public ODPEvent(String type, String action, Map<String, String> identifiers, Map<String, Object> data) {

It can support any type of data (String, Number, Boolean).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

this.type = type;
this.action = action;
this.identifiers = identifiers;
this.data = data;
}

public String getType() {
return type;
}

public void setType(String type) {
this.type = type;
}

public String getAction() {
return action;
}

public void setAction(String action) {
this.action = action;
}

public Map<String, String> getIdentifiers() {
return identifiers;
}

public void setIdentifiers(Map<String, String> identifiers) {
this.identifiers = identifiers;
}

public Map<String, String> getData() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public Map<String, String> getData() {
public Map<String, Object> getData() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return data;
}

public void setData(Map<String, String> data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public void setData(Map<String, String> data) {
public void setData(Map<String, Object> data) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

this.data = data;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* Copyright 2022, Optimizely Inc. and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.odp.serializer;

import com.optimizely.ab.odp.ODPEvent;

import java.util.List;

public interface ODPJsonSerializer {
public String serializeEvents(List<ODPEvent> events);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/**
* Copyright 2022, Optimizely Inc. and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.odp.serializer;

import com.optimizely.ab.internal.JsonParserProvider;
import com.optimizely.ab.odp.serializer.impl.GsonSerializer;
import com.optimizely.ab.odp.serializer.impl.JacksonSerializer;
import com.optimizely.ab.odp.serializer.impl.JsonSerializer;
import com.optimizely.ab.odp.serializer.impl.JsonSimpleSerializer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class ODPJsonSerializerFactory {
private static final Logger logger = LoggerFactory.getLogger(ODPJsonSerializerFactory.class);

public static ODPJsonSerializer getSerializer() {
JsonParserProvider parserProvider = JsonParserProvider.getDefaultParser();
ODPJsonSerializer jsonSerializer = null;
switch (parserProvider) {
case GSON_CONFIG_PARSER:
jsonSerializer = new GsonSerializer();
break;
case JACKSON_CONFIG_PARSER:
Comment on lines +33 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we see another set of repeated multi JSON parsers. Let's refactor them all for sharing later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its using the same JsonParserProvider to find out the default one. Its just exporting the implementation class separately. This is separate because it is a seralizer. I wanted to keep parser and serializers separate to avoid confusion. any more parsing or serialization done for ODP will go in to these same classes as added methods. Let me know what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

Separating parser and serializer looks good. I was talking about the future refactoring discussed already to reuse them in {datafile parser, event serializer, segments parser, and odp event serializer}.

jsonSerializer = new JacksonSerializer();
break;
case JSON_CONFIG_PARSER:
jsonSerializer = new JsonSerializer();
break;
case JSON_SIMPLE_CONFIG_PARSER:
jsonSerializer = new JsonSimpleSerializer();
break;
}
logger.info("Using " + parserProvider.toString() + " serializer");
return jsonSerializer;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Copyright 2022, Optimizely Inc. and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.odp.serializer.impl;

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import com.optimizely.ab.odp.ODPEvent;
import com.optimizely.ab.odp.serializer.ODPJsonSerializer;

import java.util.List;

public class GsonSerializer implements ODPJsonSerializer {
@Override
public String serializeEvents(List<ODPEvent> events) {
Gson gson = new GsonBuilder().create();
return gson.toJson(events);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* Copyright 2022, Optimizely Inc. and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.odp.serializer.impl;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.optimizely.ab.odp.ODPEvent;
import com.optimizely.ab.odp.serializer.ODPJsonSerializer;

import java.util.List;

public class JacksonSerializer implements ODPJsonSerializer {
@Override
public String serializeEvents(List<ODPEvent> events) {
ObjectMapper objectMapper = new ObjectMapper();
try {
return objectMapper.writeValueAsString(events);
} catch (JsonProcessingException e) {
// log error here
}
return null;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* Copyright 2022, Optimizely Inc. and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.odp.serializer.impl;

import com.optimizely.ab.odp.ODPEvent;
import com.optimizely.ab.odp.serializer.ODPJsonSerializer;
import org.json.JSONArray;

import java.util.List;

public class JsonSerializer implements ODPJsonSerializer {
@Override
public String serializeEvents(List<ODPEvent> events) {
return new JSONArray(events).toString();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/**
* Copyright 2022, Optimizely Inc. and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.odp.serializer.impl;

import com.optimizely.ab.odp.ODPEvent;
import com.optimizely.ab.odp.serializer.ODPJsonSerializer;
import org.json.simple.JSONArray;
import org.json.simple.JSONObject;

import java.util.List;
import java.util.Map;

public class JsonSimpleSerializer implements ODPJsonSerializer {
@Override
public String serializeEvents(List<ODPEvent> events) {
JSONArray jsonArray = new JSONArray();
for (ODPEvent event: events) {
JSONObject eventObject = new JSONObject();
eventObject.put("type", event.getType());
eventObject.put("action", event.getAction());

if (event.getIdentifiers() != null) {
JSONObject identifiers = new JSONObject();
for (Map.Entry<String, String> identifier : event.getIdentifiers().entrySet()) {
identifiers.put(identifier.getKey(), identifier.getValue());
}
eventObject.put("identifiers", identifiers);
}

if (event.getData() != null) {
JSONObject data = new JSONObject();
for (Map.Entry<String, String> dataEntry : event.getData().entrySet()) {
data.put(dataEntry.getKey(), dataEntry.getValue());
}
eventObject.put("data", data);
}

jsonArray.add(eventObject);
}
return jsonArray.toJSONString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/
package com.optimizely.ab.odp.parser;

import com.optimizely.ab.internal.JsonParserProvider;
import com.optimizely.ab.internal.PropertyUtils;
import com.optimizely.ab.odp.parser.impl.GsonParser;
import com.optimizely.ab.odp.parser.impl.JsonParser;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* Copyright 2022, Optimizely Inc. and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.odp.serializer;

import com.optimizely.ab.internal.PropertyUtils;
import com.optimizely.ab.odp.serializer.impl.GsonSerializer;
import com.optimizely.ab.odp.serializer.impl.JsonSerializer;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

import static org.junit.Assert.assertEquals;

public class ODPJsonSerializerFactoryTest {
@Before
@After
public void clearParserSystemProperty() {
PropertyUtils.clear("default_parser");
}

@Test
public void getGsonSerializerWhenNoDefaultIsSet() {
assertEquals(GsonSerializer.class, ODPJsonSerializerFactory.getSerializer().getClass());
}

@Test
public void getCorrectSerializerWhenValidDefaultIsProvided() {
PropertyUtils.set("default_parser", "JSON_CONFIG_PARSER");
assertEquals(JsonSerializer.class, ODPJsonSerializerFactory.getSerializer().getClass());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add other parsers too to avoid wrong mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Test
public void getGsonSerializerWhenGivenDefaultSerializerDoesNotExist() {
PropertyUtils.set("default_parser", "GARBAGE_VALUE");
assertEquals(GsonSerializer.class, ODPJsonSerializerFactory.getSerializer().getClass());
}
}
Loading