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.

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

fixes bug where querying by chatId results in no records #1716

Conversation

Jaredude
Copy link
Contributor

previous logic for where clause of memoryType checked for existence of chatId and would query for memoryType is null This where clause logic results in an empty result for any request to the endpoint that filters by chatId

Getting messages without any filters
Screenshot 2024-02-12 at 8 14 32 PM

filtering by known chatId value using existing where clause
Screenshot 2024-02-12 at 8 16 09 PM

filtering by known chatId value using new where clause logic for memoryType
Screenshot 2024-02-12 at 8 15 09 PM

previous logic for where clause of memoryType checked for existence of chatId and would query for memoryType is null
This where clause logic results in an empty result for any request to the endpoint that filters by chatId
also sets time window to beginning of the date or end of the date. This will be helpful with the timezone gap between the server (usually UTC) and the client (localized timezone)

Ideal date solution to consider for the future would be to adjust the timestamp query based upon the server timezone setup. This becomes particularly helpful when the client is filtering by the end date but they are in a timezone behind UTC after the UTC has advanced to the next date. For example, being in Pacific time and querying for the current date after 4PM will result in not finding records that have been created after 4PM Pacific b/c the server timestamp (in UTC) will be the next calendar date.
…if invalid startDate is passed in

also cleans up imports from typeorm lib
@Jaredude
Copy link
Contributor Author

@HenryHengZJ It turned out that the bug I thought was related to the source type was not an issue with source type at all but rather an issue with the date filter. If an invalid date was passed in for startDate or endDate, getChatMessage was not checking whether new Date() was resulting in an invalid date or not. The date picker control on the start date is defaulting to a epoch timestamp that's not compatible with new Date(), so the bug only happens when neither of the date fields are changed.

Bug (causing server crash):
https://github.com/FlowiseAI/Flowise/assets/10228950/46116b2c-1d9f-4cc3-9936-94b40a2375ea

Fixed:
https://github.com/FlowiseAI/Flowise/assets/10228950/a18b3856-9410-45a3-86bb-71c23a2f398c

date filtering working with >= <= logic instead of between to handle more robust date filtering:
https://github.com/FlowiseAI/Flowise/assets/10228950/a13cfe9c-5600-47b2-a126-f397b149fcd4

@HenryHengZJ
Copy link
Contributor

@Jaredude I'm confused how to replicate the issue, it seems to be working for me when filter by dates:
image

@Jaredude
Copy link
Contributor Author

Jaredude commented Feb 14, 2024

@HenryHengZJ Here's my steps to reproduce:

  1. yarn build
  2. yarn start
  3. go into flow
  4. view messages
  5. Filter by type

The above steps will crash the server.

Changing the start date will not result in a crash, as it will provide a date in the proper format at that point. I am able to reliably reproduce using the steps above so long as I don't change the start date first. Let me know if you're unable to reproduce using the steps above, and I'll look at things again.
video example of server crash

Edit: You can also cause a crash by passing an invalid date to the start date via the API.

@HenryHengZJ
Copy link
Contributor

@HenryHengZJ Here's my steps to reproduce:

  1. yarn build
  2. yarn start
  3. go into flow
  4. view messages
  5. Filter by type

The above steps will crash the server.

Changing the start date will not result in a crash, as it will provide a date in the proper format at that point. I am able to reliably reproduce using the steps above so long as I don't change the start date first. Let me know if you're unable to reproduce using the steps above, and I'll look at things again. video example of server crash

Edit: You can also cause a crash by passing an invalid date to the start date via the API.

Which branch are you on? I've tried multiple times on the main branch and never bumped into any issues. Filter by source works just fine

@Jaredude
Copy link
Contributor Author

I'm on main. I just did a pull, to make sure I'm up to date, and there were no files updated.

I thought the issue was filtering by source, but the issue is really with the handling of the date fields. Currently, we don't check whether new Date(startDate) or new Date(endDate) produce valid dates, so when an invalid date value gets passed into the .find method, the server will crash.

In my browser (Chrome or Safari), when I don't change the value for start date the call to new Date(startDate) is converting to "Invalid Date". This looks like it's a localization issue (possibly with the date picker library). But the server should be performing the date validation anyway since it implicitly knows the field must be a valid date to include it in the sql query.

Without changing the value of the start date the server will crash even by changing the end date and note even the type. Video of server crash on end date change without start date change.

This crash can also be produced on Postman using any invalid values for either startDate or endDate.
Screenshot 2024-02-15 at 12 11 57 PM

@HenryHengZJ
Copy link
Contributor

ah got it, successfully replicated it through postman! thanks for the fix @Jaredude !!

@HenryHengZJ HenryHengZJ merged commit e4cc333 into FlowiseAI:main Feb 15, 2024
2 checks passed
@Jaredude
Copy link
Contributor Author

Whew! 😅I was starting to worry something was wrong with my laptop!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants