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

fix for wrong date values #4554

Merged
merged 7 commits into from
Jun 7, 2024
Merged

Conversation

ArqamFarooqui110719
Copy link
Contributor

fix for wrong date values issue #4541

@ArqamFarooqui110719
Copy link
Contributor Author

I have checked that one test is failing because in that FT invalid date value (017-06-17T07:21:24.238Z) has been given.
In functionalTest/cases/1087_error_handling/error_messages_for_invalid_tenant_in_v2.test FT file at L#44, valid date is not used, see below FT for reference:

echo "01. POST /v2/entities with invalid tenant"
echo "========================================="
payload='{
  "type": "room",
  "id": "room_0",
  "timestamp_0":
  {
    "type": "DateTime",
    "value": "017-06-17T07:21:24.238Z",
    "metadata": {
      "very_hot_1":
      {
        "type": "alarm",
        "value": "Afe1BuMTcf"
      },
      "very_hot_0":
      {
        "type": "alarm",
        "value": "Afe1BuMTcf"
      }
    }
  }
}

So I think we should update this FT with valid date (2017-06-17T07:21:24.238Z). What is your opinion on this @fgalan?

@fgalan
Copy link
Member

fgalan commented May 28, 2024

So I think we should update this FT with valid date (2017-06-17T07:21:24.238Z). What is your opinion on this @fgalan?

I agree.

Please do that in this PR. Thanks!

Comment on lines +621 to +623
if (time.tm_year < minYear || time.tm_mon < minMonth || time.tm_mon > maxMonth || time.tm_mday < minDay ||
time.tm_mday > maxDay || time.tm_hour < minHour || time.tm_hour > maxHour || time.tm_min < minMinute ||
time.tm_min > maxMinute || time.tm_sec < minSecond || time.tm_sec > maxSecond)
Copy link
Member

Choose a reason for hiding this comment

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

What about 2024-02-30? As far as I understand, this will pass this check, but February 30th is not a valid date ;)

Maybe you should make a function maxDay(month, year) which return the maxDay based on month and year. Basically:

  • January: 31
  • February: 29 in case of leap-year, 28 otherwise. Maybe you need an additional isLeapYear(year) function for this (maybe this reference is useful to implement it https://www.geeksforgeeks.org/program-check-given-year-leap-year)
  • March: 31
  • April: 30
  • May: 31
  • June: 30
  • July: 31
  • August: 31
  • September: 30
  • October: 31
  • November: 30
  • December: 31

Please include 2024-02-29 (should be correctly accepted) and 2023-02-29 (should return error) in your tests to check it works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1a824bf

@ArqamFarooqui110719
Copy link
Contributor Author

Don't know why UT is failing after syncing(rebasing) the branch (issue/4541).

@fgalan
Copy link
Member

fgalan commented Jun 6, 2024

Don't know why UT is failing after syncing(rebasing) the branch (issue/4541).

I would have a look to it

Comment on lines 546 to 554
if (year % 4 != 0)
{
return false;
}
if (year % 100 == 0 && year % 400 != 0)
{
return false;
}
return true;
Copy link
Member

@fgalan fgalan Jun 6, 2024

Choose a reason for hiding this comment

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

I thinks is clearer in "positive" way:

Suggested change
if (year % 4 != 0)
{
return false;
}
if (year % 100 == 0 && year % 400 != 0)
{
return false;
}
return true;
// ref: https://www.geeksforgeeks.org/program-check-given-year-leap-year/
if (year % 400 == 0)
{
return true;
}
if ((year % 4 == )0 && (year % 100 != 0))
{
return true;
}
return false;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in d2e95bc

@fgalan
Copy link
Member

fgalan commented Jun 6, 2024

Don't know why UT is failing after syncing(rebasing) the branch (issue/4541).

I would have a look to it

It has been solved in PR #4577. Please sync this PR branch with master.

"pressure"
]
},
"expires": "1822-12-21T17:16:23.00Z"
Copy link
Member

Choose a reason for hiding this comment

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

This is a year in XIX century :), but from a point of view of the checking logic 1822 is greater than minYear = 0;.

So why is this an invalid year? Maybe I'm missing something...

Copy link
Contributor Author

@ArqamFarooqui110719 ArqamFarooqui110719 Jun 7, 2024

Choose a reason for hiding this comment

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

In this test case, I am checking 'correct value for year field'. As per existing code in globals.cpp file

  struct tm time;
  time.tm_year = y - 1900; // Year since 1900
  time.tm_mon  = M - 1;    // 0-11
  time.tm_mday = d;        // 1-31
  time.tm_hour = h;        // 0-23
  time.tm_min  = m;        // 0-59
  time.tm_sec  = (int) s;  // 0-61 (0-60 in C++11)

In above code, The base year has been set to 1900. Therefore, any year earlier than 1900 should be considered invalid. In that context, I have randomly chosen the year 1822 as an invalid date in the test case.

Copy link
Contributor Author

@ArqamFarooqui110719 ArqamFarooqui110719 Jun 7, 2024

Choose a reason for hiding this comment

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

but from a point of view of the checking logic 1822 is greater than minYear = 0;.

In the existing code, the original year value is subtracted by 1900.
so 1822 or any value lesser than year 1900 is actually "less than 0". (1899-1900)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I didn't remember that substraction :) Makes sense

Maybe this should be warned in the new code (I provide a suggestion). With regards to this comment thread, NTC

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be warned in the new code (I provide a suggestion)

Not really needed... looking the code in context, it's clear enough as it is now.

"pressure"
]
},
"expires": "2024-02-29T25:16:11.00Z"
Copy link
Member

Choose a reason for hiding this comment

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

I think this test is failing due to hour (25) not year, as 2024 is leap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 3d2a20e

Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

@fgalan fgalan merged commit 17bb087 into telefonicaid:master Jun 7, 2024
12 checks passed
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