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

Convert DateTimeOffset to DateTime when necessary #584

Merged
merged 1 commit into from Dec 20, 2018

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Jul 11, 2017

Issues

This pull request fixes issue #578.

Description

Entities with a DateTime property in their key can't be created/read/updated/deleted because OData Web API does not support DateTime in the keys. This pull request addresses this issue by converting DateTimeOffset to DateTime when necessary.

Checklist

  • Test cases added
  • Build and test with one-click build and test script passed

Additional work necessary

No additional work necessary.

@xuzhg
Copy link
Member

xuzhg commented Jul 12, 2017

@0xced Would you please add several tests to cover your change-sets?

@rayao
Copy link
Contributor

rayao commented Jul 15, 2017

ChangeSetItem and RestierQueryBuilder are at 2 different level, one is in Core and the other publisher, we'd better make the change more consistent.

@0xced
Copy link
Contributor Author

0xced commented Jul 29, 2017

Would you please add several tests to cover your change-sets?

@xuzhg As I explained, this problem only exists when a DateTime is part of the primary key and I could not find a part of the sample databases used in the tests with a DateTime as part of the primary key. If there’s one I could probably write a test but I don't feel like creating a whole new database just for this. :-/

we'd better make the change more consistent

@rayao What would you suggest to make the change more consistent? I thought that extracting the logic in a helper class was precisely making it consistent.

@OData OData deleted a comment from msftclas Sep 27, 2017
@OData OData deleted a comment from msftclas Sep 27, 2017
@0xced
Copy link
Contributor Author

0xced commented Dec 4, 2017

Ping.

@rayao
Copy link
Contributor

rayao commented Dec 7, 2017

I'm ok with this change.

@robertmclaws
Copy link
Collaborator

@0xced I'd like to be able to accept this PR, but you're going to need to catch up on your branch before I can pull it in. Thanks!

This is required for keys with a DateTime property

Fixes OData#578
@0xced
Copy link
Contributor Author

0xced commented Dec 20, 2018

I just rebased on master. It seems the continuous integration has failed but unfortunately I'm not authorized to see what has failed.

@robertmclaws robertmclaws merged commit af7ad3f into OData:master Dec 20, 2018
@robertmclaws
Copy link
Collaborator

Don't worry about the CI, I broke the project by moving it to an entirely new structure, so it's going to keep failing until Microsoft gives me access to fix things. I just wanted to make sure the code was current and still necessary so I could pull it in. Approving now, and I hope to have a CI build you can you before the end of the weekend. Thanks for your contribution!

@0xced 0xced deleted the DateTime-key branch December 20, 2018 16:50
robertmclaws added a commit that referenced this pull request Dec 19, 2022
Convert DateTimeOffset to DateTime when necessary
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.

None yet

6 participants