Fix date-handling bug when today’s date is later than the target month

43 points by dsiegel2275 1 year ago | 59 comments
  • MrHus 1 year ago
    I was just browsing HN because I'm procrastinating solving a date based bug. It is literally this bug! Who said surfing HN is not productive!
  • torstenvl 1 year ago
    How is 3+3+2+3+2+3+3+2+3+2+3 equal to 5?

    The bug exists every time today's date is greater than the lowest total number of days in a month (28). So it's a bug for 3 days in January, (sometimes 1 day in February,) 3 days in March, 2 days in April, etc.

    • steanne 1 year ago
      in a SPECIFIC month. it's only 28 for february, and then only sometimes. it's only going to affect months where the month contains more days than the month after it. january, march, may, august, and october.

      of those, however, there's more than one date in january which would error, so the total is more than 5. 6 or 7 depending on if it's a leap year

      • torstenvl 1 year ago
        The bug occurs when using setMonth() to a month value containing fewer days than the current date.

        There is no limitation in setMonth() such that you can only set the month to "the month after."

        I'm sorry, but I think you misunderstand this bug.

        • Izkata 1 year ago
          I think you did too, I don't have the slightest clue where your math came from. This bug happens when the month you're setting it to has fewer days than the month in the date object, so it rolls the day forward into the next month. It's the same functionality that lets you just add days and iterate over the whole year.
    • jakub_g 1 year ago
      Very relatedly, there's a potential similar footgun related to hours, due to the fact that dates initialized without hour default to 00:00, and that JS dates are in user's current local timezone.

      It's possible then that after changing the month programmatically, the time component will jump to 23:00 due to timezone DST changes, and the date will jump back one day as well due to this (and in an unlucky case, the month will jump back as well!)

      Example bug I had in 2014 when Russian timezone definitions have changed:

      https://github.com/yui/yui2/pull/15

      A workaround is to initialize dates with fake "midday" time component so that you avoid the day jumping back.

      • 1 year ago
      • tzs 1 year ago
        The fix is to change things like this:

          targetEndDate = new Date();
          targetEndDate.setFullYear( endDate.getFullYear() );
          targetEndDate.setMonth( endDate.getMonth() );
          targetEndDate.setDate( endDate.getDate() );
        
        to

          targetEndDate = new Date();
          targetEndDate.setFullYear(
            endDate.getFullYear(),
            endDate.getMonth(),
            endDate.getDate());
        
        That works because setFullYear has a three argument form that takes the year, month, and date avoiding the inconsistency that can arise by setting those one at a time.

        But you know what else has a three argument for that take the year, month, and date? The Date constructor.

        So why not fix it like this?

          targetEndDate = new Date(
            endDate.getFullYear(),
            endDate.getMonth(),
            endDate.getDate());
        
        Using the empty constructor would initial the object with the current date and time, but they promptly overwrite those.

        The Date construction also has a form that takes another Date object, so I wonder if they could have simple used:

          targetEndDate = new Date(endDate);
        • solardev 1 year ago
          You should never, ever do date math naively like this. There are too many unexpected edge cases, especially between time zones or daylight savings time or leap years, but even without them: https://moment.github.io/luxon/#/math

          In fact I would strongly argue you should never use the JS Date built-ins at all because they are terrible. Use a library like Luxon or date-fns. As a frontend dev, this is the most common category of bugs I've dealt with in my career, and I've spent dozens of hours fixing other people's datetime handling because of how poorly implemented the JS Date API is. It's full of minefields and gotchas.

          The Temporal API is supposed to fix some issues, but that's been in development for like a decade now.

          • avandekleut 1 year ago
            Reminder to use something like the date-fns library for immutable operations on dates. The native JS date library can be painful to work on when doing operations like adding or subtracting time.
            • wdfx 1 year ago
              I would argue that doing time arithmetic by modifying components of a date individually is wrong in any language.
              • JKCalhoun 1 year ago
                A function that takes all the components at once can resolve this conundrum internally.

                A function that takes a simple struct (containing each component) can also handle the conundrum. The function could of course even take a string representation.

                • djrockstar1 1 year ago
                  And the right way would be?
                  • wdfx 1 year ago
                    To have a better understanding of what operations you are trying to do. I can't answer your question specifically because it depends a lot on use case and what date/time standards you want to use.
                    • solardev 1 year ago
                      To be honest, use a library where someone else figured out the ambiguities and accounted for the edge cases. Good starting point: https://moment.github.io/luxon/#/math

                      Date-fns is fine for simpler use cases but Luxon is a lot more complete, especially where it comes to time zones.

                      This is not the kind of thing you just want to blindly hack your way through without a really thorough understanding of how different cultures/locales/governments handle dates and times.

                      If you have to do math across daylight savings time boundaries that change over time (because governments and regulations change), converted to two or more time zones (which again have their own separate DST rules), and possibly span some 30 or 45 minute time zones (they exist!) this will quickly get out of hand and unreadable. Not just unreadable, but incredibly difficult to reason about.

                      It gets even worse when the backend stores only the offset (as in the ISO time, like T23:00:00+08:00) because then you lose the original time zone and can't be sure whether it came from a DST zone or another country in the same zone (but only during part of the year).

                      • JR1427 1 year ago
                        Convert to unix times, and do the calculation with those?
                    • angrygoat 1 year ago
                      Temporal is quite nice; should be coming to browsers soon: https://tc39.es/proposal-temporal/
                      • wayvey 1 year ago
                        Nice! JS has always been awkward with dates, good to see that is being fixed on the language level.
                        • solardev 1 year ago
                          they've been saying that for years now
                        • tantalor 1 year ago
                          What does mutability have to do with this bug?
                          • hyperpape 1 year ago
                            The old code was

                                targetEndDate = new Date();
                                targetEndDate.setFullYear(endDate.getFullYear());
                                targetEndDate.setMonth(endDate.getMonth());
                                targetEndDate.setDate(endDate.getDate());
                            
                            Even if the endDate is valid, this can fail, because today is the 31st, which doesn't exist in endDate's month.

                            If you just did:

                                targetEndDate = createDate(endDate.getFullYear(), endDate.getMonth(), endDate.getDate()); 
                                // createDate is an intentional placeholder, I didn't want to double-check the actual syntax
                            
                            you wouldn't pass through the transitional stage where you have the new month but today's day of the month, which is what causes the bug.

                            In this case, using mutation on the individual fields makes you have to transition through an invalid state to get back to a valid one, and the JS date object does something unexpected (though I think there's nothing good to do here, an exception is probably the best you could do). So mutation really is at issue here. In general, mutation creates room for these kind of counter-intuitive state transitions to arise.

                            • rawling 1 year ago
                              The direct "immutable" equivalent of the bug code (purely based on my experience with .NET Core immutable collections) would have the setX methods return a new immutable datetime with the field set as requested. So just "use an immutable type" wouldn't fix this bug.

                              "Change the code completely so you supply all 3 components at the same time" obviously fixes the bug, but that doesn't require an immutable type.

                              • magicalhippo 1 year ago
                                > mutation on the individual fields makes you have to transition through an invalid state

                                If you want to do this mutation-style for some reason, you should use the builder pattern (or whatever it's called). So in this case you'd instantiate a DateBuilder instance instead of a Date instance, and finally call .date() to get the actual Date instance from the builder instance.

                                • macintux 1 year ago
                                  I agree, reading the example code was painful as someone who tries to write all code in a functional style.
                            • wouldbecouldbe 1 year ago
                              I make this bug in my taxes every now and then. Most invoices are from EU, but the few US ones have the months in wrong order, I only notice if the days exceed 12
                              • wheybags 1 year ago
                                This is why I normally use "23/Aug/2023" form. Only breaks down if you don't know the months in English.
                                • a_gnostic 1 year ago
                                  My military friend uses YYYY/MM/DD. Used to tick me off, but makes sense now.
                                  • lnx01 1 year ago
                                    ISO8601 or death
                                    • fragmede 1 year ago
                                      ISO 8601 is yyyy-mm-ddthh:mm:ss.sssZ
                                      • DarkmSparks 1 year ago
                                        I use this format generally because a basic string sort puts them in the correct order.

                                        trying to sort MM/DD/YYYY is just silly.

                                        • thfuran 1 year ago
                                          Days in that format even order as proper numbers instead of some kind of little endian disaster.
                                          • bryanlarsen 1 year ago
                                            I've seen people do YYYY/DD/MM, so your friend's format isn't foolproof. OTOH although I'm sure some insane person somewhere has done YYYY-DD-MM I've never seen it so YYYY-MM-DD should be safe.
                                            • jayknight 1 year ago
                                              ISO 8601, ftw.
                                            • pimlottc 1 year ago
                                              I also really like this form for regular communications, though I use spaces. It feels more natural than ISO format while being unambiguous. You can also add the day of the month and it's nicely balanced between letters and numbers, and you can also drop the year when it's clear from context:

                                              "Hey, are you free for dinner on Wed 23 Aug?"

                                              ISO is great for records and filename, but it's clumsy in conversation:

                                              "Hey, are you free for dinner on 2023-08-20?"

                                          • RicoElectrico 1 year ago
                                            Apparently, date handling is still causing problems in 2024, as evidenced by the Newag trains' DRM.
                                            • oleganza 1 year ago
                                              Unlike many issues with dates, this one is purely a result of a wrong API design. It does not make sense to "set a component" of a date all by itself (unlike hours/minutes/seconds in a day), so an API that allows you to so is plain broken.

                                              Correct API would be to set the components D,M,Y all at once and have them validated on the spot before returning a valid Date value.

                                              • candiddevmike 1 year ago
                                                I had something like this for a while. It wasn't a bug per-say, it was a test flake due to dates and follow on calculations based on them. On the plus side, it prevented you from working on the weekend as the tests/PRs would always fail during that time.
                                              • wffurr 1 year ago
                                                Did I not look closely enough? I didn’t see any tests in the changes.
                                                • Pxtl 1 year ago
                                                  I've seen somebody write this exact bug before! Hah. Like, something to the effect of

                                                      var newDate = new Date(GetDate().Year, GetDate().Month + 1, GetDate().Day);
                                                  
                                                  but with extra logic to ensure that the Month component wrapped around.
                                                  • Izkata 1 year ago
                                                    > but with extra logic to ensure that the Month component wrapped around

                                                    Extra unnecessary logic. The functionality that causes this bug is so you don't need to handle wraparound yourself:

                                                      >> new Date(2023, 12, 1)
                                                      Date Mon Jan 01 2024 00:00:00 GMT-0600 (Central Standard Time)
                                                    
                                                    (Note months are 0-indexed so 12 is one past the last month of the year)

                                                    It works with negative numbers too, to wrap backwards:

                                                      >> new Date(2024, -1, 1)
                                                      Date Thu Dec 01 2023 00:00:00 GMT-0600 (Central Standard Time)
                                                    • Pxtl 1 year ago
                                                      Depends on the language/library. C#'s DateTime will throw ArgumentOutOfRangeException.
                                                  • cranium 1 year ago
                                                    I tried to replicate the bug only to find out the months are 0-indexed (jan = 0) so the example should use `setMonth(1)`.

                                                    I must say I'd prefer getting an exception rather than a silent roll-over any day of the week – pun intended.

                                                    • 1 year ago
                                                      • ramses0 1 year ago
                                                        ...the comment! The hubris!

                                                                // Important: Important to set these in order
                                                        • chmod775 1 year ago
                                                          Yeah, there's multiple variations of that bug possible depending on what you set first, so setting these in that specific order likely avoided one bug they noticed.

                                                          They just didn't realize they have a different bug now.

                                                        • 1 year ago