Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

Crud status validation #1117

Merged
merged 132 commits into from Mar 18, 2019
Merged

Crud status validation #1117

merged 132 commits into from Mar 18, 2019

Conversation

StaticallyTypedAnxiety
Copy link
Contributor

@StaticallyTypedAnxiety StaticallyTypedAnxiety commented Mar 13, 2019

  • I have added a summary of my changes to the changelog
  • added validation for update
  • added validation for delete
  • modified macros for Entry Validation
  • modified macros for LinkValidation
  • added app_spec tests for Validation

note : LinkValidation and EntryValidation had to be seperate because you do not want the user to check entry variants for Link data and you do not want the user to check Link Validations for Entries(even though links are technically entries). On top of that you cannot modify links

@StaticallyTypedAnxiety StaticallyTypedAnxiety changed the title [WIP]Crud status validation Crud status validation Mar 15, 2019
core/src/nucleus/validation/app_entry.rs Outdated Show resolved Hide resolved
validation_package
))
} else {
await!(run_call_back(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This call is exactly the same as above, no? Could be pulled out of the if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I will make changes to this

let entry = get_entry_from_dht(&context.clone(), address)?.ok_or(
HolochainError::ErrorGeneric("Could not get Entry".to_string()),
)?;
let headers = state.get_headers(address.clone())?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this right: you're getting the entry from the DHT but then assert that the header is already in the local state. This actually might be the case right now since we still have a full-sync DHT, but with this you would actually introduce the first bit in core that depends on a full-sync scenario, which I don't think we should do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed this will be moved into a separate branch as this code assumes a full synced DHT

core/src/nucleus/validation/remove_entry.rs Outdated Show resolved Hide resolved
core_types/src/validation.rs Show resolved Hide resolved
Nicolas Luck and others added 5 commits March 18, 2019 10:43
Co-Authored-By: AshantiMutinta <AshantiMutinta@gmail.com>
Co-Authored-By: AshantiMutinta <AshantiMutinta@gmail.com>
Copy link
Collaborator

@lucksus lucksus left a comment

Choose a reason for hiding this comment

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

Cool 👍

Copy link
Member

@zippy zippy left a comment

Choose a reason for hiding this comment

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

This looks great, just one stray println.

@@ -34,6 +34,7 @@ pub fn fetch_entry_with_header(
context: &Arc<Context>,
) -> Result<EntryWithHeader, HolochainError> {
let entry = fetch_entry_from_cas(address, &context)?;
println!("entry found {:?}", entry.clone());
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be a call to context.log() or deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes I thought I got them all haha. Will make those changes thank you so so much

/// test that a commit will publish and entry to the dht of a connected instance via the in-memory network
fn test_commit_with_dht_publish() {
let mut dna = test_dna();
dna.uuid = "test_commit_with_dht_publish".to_string();
let netname = Some("test_commit_with_dht_publish, the network");
let (_instance1, context1) = instance_by_name("jill", dna.clone(), netname);
let (_instance2, context2) = instance_by_name("jack", dna, netname);

//let test = JsonString::from(RawString::from("stuff:\"))
Copy link
Member

Choose a reason for hiding this comment

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

stale comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stale comment :)

@StaticallyTypedAnxiety StaticallyTypedAnxiety merged commit c01fc4a into develop Mar 18, 2019
@Connoropolous Connoropolous mentioned this pull request Mar 21, 2019
35 tasks
@zippy zippy deleted the crud_status_validation branch July 5, 2019 13:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants