Skip to content

Add functionality to disable forwarding per device #5351

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
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add functionality to disable forwarding per device #5351

wants to merge 11 commits into from

Conversation

mivtdole
Copy link

This is the rewrite of and old pull request

@tananaev
Copy link
Member

Where's the old pull request? Provide a link.

@tananaev tananaev mentioned this pull request Jun 19, 2024
Open
@mivtdole
Copy link
Author

See closed pull request #4778

@tananaev
Copy link
Member

What is the use case for this? It looks like you originally has "allow forwarding". That actually makes more sense, but probably need another configuration parameters for it. We should also probably do the same thing for event forwarding.

@mivtdole
Copy link
Author

I created this version because of your comment "That's why I'm thinking that the attribute logic should be inversed. You add it to disable forwarding." in the previous pull request.

@tananaev
Copy link
Member

I know what I said, but I said it because you only had one parameter, which would break existing logic. But if we add another configuration option, it would be fine.

@mivtdole
Copy link
Author

I know what I said, but I said it because you only had one parameter, which would break existing logic. But if we add another configuration option, it would be fine.

What do you mean by other configuration option?

@tananaev
Copy link
Member

I mean a config parameter that will enable forwarding only for marked devices.

@mivtdole
Copy link
Author

I will make an updated version with a config parameter

@mivtdole
Copy link
Author

Do you want a new pull request or an update to this one?

@tananaev
Copy link
Member

Please use the same pull request. That's always the preference, so we have full conversation here.

@mivtdole
Copy link
Author

Just to be clear:

  • We want a new configuration option with which the system manager can specify if per device forwarding needs to be enabled? Shall we call this configuration option "forward.enablePerDevice" and make it boolean. When this option is missing or false the "old" normal behavior of data for every device is forwarded. Not looking at any forwarding attribute for the device.
  • When the configuration option is true. If the user wants to forward data for a device they need to add the attribute "allowForwarding" for the device.

@tananaev
Copy link
Member

Yes, correct.

@mivtdole
Copy link
Author

Changes pushed. Added config and attribute options for position and event forwarding. Hope you like it.

tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
src/main/java/org/traccar/database/NotificationManager.java Outdated
@@ -80,6 +84,8 @@ public NotificationManager(
blockedUsers.add(Long.parseLong(userIdString));
}
}

this.perDevice = config.getBoolean(Keys.EVENT_FORWARD_ENABLE_PER_DEVICE);
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary this.

tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
src/main/java/org/traccar/database/NotificationManager.java Outdated
LOGGER.warn("Event forwarding failed", throwable);
Device device = cacheManager.getObject(Device.class, event.getDeviceId());

Object allowForwarding = device.getAttributes().get(ATTRIBUTE_DEVICE_ALLOW_FORWARDING);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this an object? Use getBoolean.

Copy link
Author

Choose a reason for hiding this comment

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

Changed it to

boolean allowForwarding = (boolean) device.getAttributes().get(ATTRIBUTE_DEVICE_ALLOW_FORWARDING);

tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
tananaev
tananaev reviewed Jun 27, 2024
View reviewed changes
tananaev
tananaev reviewed Jul 1, 2024
View reviewed changes
src/main/java/org/traccar/database/NotificationManager.java
LOGGER.warn("Event forwarding failed", throwable);
Device device = cacheManager.getObject(Device.class, event.getDeviceId());

boolean allowForwarding = (boolean) device.getAttributes().get(ATTRIBUTE_DEVICE_ALLOW_FORWARDING);
Copy link
Member

Choose a reason for hiding this comment

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

There should be a getBoolean you can use.

tananaev
tananaev reviewed Jul 1, 2024
View reviewed changes
src/main/java/org/traccar/handler/PositionForwardingHandler.java
new AsyncRequestAndCallback(positionData).send();
Device device = cacheManager.getObject(Device.class, position.getDeviceId());

boolean allowForwarding = (boolean) device.getAttributes().get(ATTRIBUTE_DEVICE_ALLOW_FORWARDING);
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

tananaev
tananaev reviewed Jul 1, 2024
View reviewed changes
src/main/java/org/traccar/database/NotificationManager.java
Device device = cacheManager.getObject(Device.class, event.getDeviceId());

boolean allowForwarding = (boolean) device.getAttributes().get(ATTRIBUTE_DEVICE_ALLOW_FORWARDING);
if (!enablePerDevice || (enablePerDevice && allowForwarding)) {
Copy link
Member

Choose a reason for hiding this comment

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

This check is redundant. You can do !enablePerDevice || allowForwarding.

tananaev
tananaev reviewed Jul 1, 2024
View reviewed changes
src/main/java/org/traccar/handler/PositionForwardingHandler.java
Device device = cacheManager.getObject(Device.class, position.getDeviceId());

boolean allowForwarding = (boolean) device.getAttributes().get(ATTRIBUTE_DEVICE_ALLOW_FORWARDING);
if (!enablePerDevice || (enablePerDevice && allowForwarding)) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

tananaev
tananaev reviewed Jul 1, 2024
View reviewed changes
src/main/java/org/traccar/database/NotificationManager.java
}
});
} else {
LOGGER.debug("Forwarding events not enabled for {}", event.getDeviceId());
Copy link
Member

Choose a reason for hiding this comment

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

It should be unique id, not device id.

tananaev
tananaev reviewed Jul 1, 2024
View reviewed changes
src/main/java/org/traccar/handler/PositionForwardingHandler.java
positionData.setDevice(device);
new AsyncRequestAndCallback(positionData).send();
} else {
LOGGER.debug("Forwarding position not enabled for {}.", position.getDeviceId());
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tananaev tananaev tananaev left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mivtdole @tananaev