Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Send packet only with POST method. #470

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Send packet only with POST method. #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 3 additions & 19 deletions tracker/src/main/java/org/matomo/sdk/dispatcher/PacketFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@

package org.matomo.sdk.dispatcher;

import androidx.annotation.NonNull;
import android.text.TextUtils;

import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import android.text.TextUtils;

import org.json.JSONArray;
import org.json.JSONException;
Expand All @@ -37,19 +37,11 @@ public PacketFactory(final String apiUrl) {
public List<Packet> buildPackets(final List<Event> events) {
if (events.isEmpty()) return Collections.emptyList();

if (events.size() == 1) {
Packet p = buildPacketForGet(events.get(0));
if (p == null) return Collections.emptyList();
else return Collections.singletonList(p);
}

int packets = (int) Math.ceil(events.size() * 1.0 / PAGE_SIZE);
List<Packet> freshPackets = new ArrayList<>(packets);
for (int i = 0; i < events.size(); i += PAGE_SIZE) {
List<Event> batch = events.subList(i, Math.min(i + PAGE_SIZE, events.size()));
final Packet packet;
if (batch.size() == 1) packet = buildPacketForGet(batch.get(0));
Copy link
Collaborator

@hannesa2 hannesa2 Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@d4rken
When I understand right, you did in 1f8ccd6 this decision if it's a get or a post by asking batch.size(), am I right ?

Do you agree with these changes ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember why I did it this way 🤔.

Might have been an optimization, i.e. GET having less overhead than POST for single events. In hindsight I don't think the overhead would be huge enough to warrant both GET and POST.

I'm neutral on this change.

@nostromoo Please consider adding some descriptions to your PRs. What made you change this?

else packet = buildPacketForPost(batch);
final Packet packet = buildPacketForPost(batch);
if (packet != null) freshPackets.add(packet);
}
return freshPackets;
Expand All @@ -74,12 +66,4 @@ private Packet buildPacketForPost(List<Event> events) {
}
return null;
}

// "http://domain.com/matomo.php?idsite=1&url=http://a.org&action_name=Test bulk log Pageview&rec=1"
@Nullable
private Packet buildPacketForGet(@NonNull Event event) {
if (event.getEncodedQuery().isEmpty()) return null;
return new Packet(mApiUrl + event);
}

}
17 changes: 1 addition & 16 deletions tracker/src/test/java/org/matomo/sdk/dispatcher/PacketFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,6 @@ public void testPOST_data() throws Exception {
assertEquals("berries", packets.get(0).getPostData().getJSONArray("requests").get(1));
}

@Test
public void testGET_apiUrl() {
String url = "http://example.com/";
PacketFactory factory = new PacketFactory(url);
List<Packet> packets = factory.buildPackets(Collections.singletonList(new Event("strawberries")));
assertTrue(packets.get(0).getTargetURL().startsWith(url));
}

@Test
public void testGET_badUrl() {
PacketFactory factory = new PacketFactory("http://example.com/");
assertTrue(factory.buildPackets(Collections.singletonList(new Event(""))).isEmpty());
}

@Test
public void testEmptyEvents() {
PacketFactory factory = new PacketFactory("http://example.com/");
Expand All @@ -71,8 +57,7 @@ public void testPacking_rest() {

Packet second = packets.get(1);
assertEquals(1, second.getEventCount());
assertNull(second.getPostData());
assertTrue(second.getTargetURL().endsWith("?eve" + events.size()));
assertNotNull(second.getPostData());
}

@Test
Expand Down
Loading