Skip to content

Conversation

@DerDreschner
Copy link
Collaborator

Generating shuffled samples is based on IP addresses which are used exclusively by one user. If no user has such an unique IP address, the returned DataSet object contains no IP address at all. This results in an exception during merge with the "regular" DataSet objects due to the different length.

To mitigate this, I now return an empty Labeled() object which can be used in the merge function of RubixML without any issues. Additionally, I revert the change from #810 as generateFromRealData shouldn't be called without any unique IPs at all. I suspect that all people in #745 are affected by this bug in the first place and experience the error from #860 now.

This should fix the latest reports in #860.

@DerDreschner DerDreschner force-pushed the fix/860-error-on-merge-when-no-shuffle-possible branch from 320fc50 to 8acd395 Compare August 20, 2025 15:42
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Makes sense! Thanks a lot!

$max = count($positives);
$uniqueIps = $this->getUniqueIPsPerUser($positives);

if(empty($uniqueIps)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(empty($uniqueIps)) {
if ($uniqueIps === []) {

cs

@ChristophWurst ChristophWurst added bug Something isn't working 3. to review labels Aug 20, 2025
@ChristophWurst
Copy link
Member

/backport to stable31

@ChristophWurst
Copy link
Member

/backport to stable30

@ChristophWurst
Copy link
Member

Please squash your commits into one fix: xxx commit after the coding style clean-up :)

@DerDreschner DerDreschner force-pushed the fix/860-error-on-merge-when-no-shuffle-possible branch from e85aa90 to fbd29be Compare August 22, 2025 16:46
@DerDreschner
Copy link
Collaborator Author

Please squash your commits into one fix: xxx commit after the coding style clean-up :)

Done! :)

auto-merge was automatically disabled August 26, 2025 18:05

Head branch was pushed to by a user without write access

@DerDreschner DerDreschner force-pushed the fix/860-error-on-merge-when-no-shuffle-possible branch from 9ccf606 to 49bbdbf Compare August 26, 2025 18:11
@DerDreschner
Copy link
Collaborator Author

@ChristophWurst : I've run composer run cs:fix and commited the changes as this pipeline run failed. Afterwards, I've squashed everything into one commit again.

Unfortunately, the static code analysis fails as well due to deprecation warnings in the voku/portable-utf8 library (please see my new PR with a fix for that).

Could you please run the workflows again and mitigate the issue with the failed static code analysis run? Thanks!!

@github-actions
Copy link

github-actions bot commented Sep 4, 2025

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@DerDreschner
Copy link
Collaborator Author

@ChristophWurst : Sorry to bother you again, but I hit the "re-request review" button and now it doesn't merge automatically, although all workflows are green now. Could you do it one more time? Thank you! 🙈

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

:)

@ChristophWurst
Copy link
Member

I hit the "re-request review" button and now it doesn't merge automatically, although all workflows are green now

Apparently auto merge stops for forks once you push another time

Head branch was pushed to by a user without write access

@ChristophWurst ChristophWurst merged commit 1bd411b into nextcloud:master Sep 9, 2025
31 checks passed
@ChristophWurst
Copy link
Member

/backport to stable32

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants