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

Environment layer #52

Merged
merged 5 commits into from
Sep 12, 2017
Merged

Environment layer #52

merged 5 commits into from
Sep 12, 2017

Conversation

clenk
Copy link
Contributor

@clenk clenk commented Sep 8, 2017

Open questions:

  • Should each Environment object keep track of its own mapping of custom object types and classes? The current implementation does not.
  • The stix2 validator is currently not run on data in any of the datastore implementations. Should this be brought back? One thought is that the API should make it difficult to create invalid STIX anyway, so this would be redundant. On the other hand, the API may not catch all errors, and trying to do so may result in duplicate code.

Closes #37

The Environment layer does not keep its own mapping of custom object types.
If we think it likely that users will want to maintain separate lists of
custom object types between two or more Environments, we can add this
later.
1) 'stix_data' is now optional; you can set up a MemorySink without
needing a starting set of data.
2) Removed stix2-validator calls. The validator fails when called on our
_STIXBase-derived classes because we store timestamps as datetime
objects, while the validator expects strings. Also, this was the only
datastore that used the validator, and we should be consistent across
all of them. The validator can be added back in later.
@codecov-io
Copy link

codecov-io commented Sep 8, 2017

Codecov Report

Merging #52 into master will increase coverage by 0.72%.
The diff coverage is 98.59%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #52      +/-   ##
========================================
+ Coverage   95.27%    96%   +0.72%     
========================================
  Files          53     53              
  Lines        4506   4626     +120     
========================================
+ Hits         4293   4441     +148     
+ Misses        213    185      -28
Impacted Files Coverage Δ
stix2/core.py 100% <ø> (ø) ⬆️
stix2/environment.py 100% <100%> (ø) ⬆️
stix2/sources/__init__.py 96.96% <100%> (+0.19%) ⬆️
stix2/__init__.py 100% <100%> (ø) ⬆️
stix2/test/test_environment.py 100% <100%> (ø) ⬆️
stix2/sources/memory.py 83.56% <77.77%> (+7.65%) ⬆️
stix2/sources/filesystem.py 22.22% <0%> (+22.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adac437...190b639. Read the comment docs.

return self.source.get(*args, **kwargs)
except AttributeError:
raise AttributeError('Environment has no data source to query')
get.__doc__ = DataStore.get.__doc__
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like these should use the DataSource docstrings, but after taking a look at those, they could probably be cleaned up and reorganized between DataStore and DataSource. It's fine for now. I created #53 to address that.

@gtback
Copy link
Contributor

gtback commented Sep 11, 2017

My first impression is that keeping track of custom types should be an Environment function. A user could conceivably want different settings in different parts of your application, which is the purpose of the Environment class. If we decide that we want to enable custom types outside of an explicit environment (which I'm not sure we do), that means we need to keep an implicit environment for that. The core parse function would then call the parse function for that environment. An implicit environment is also a step towards the "Workbench" API, so maybe we just defer the "implicit custom types" to that level, and require an explicit Environment otherwise.

I'm fine with not running the validator for every DataSource/DataSink interaction. If it's needed, we can later add a boolean validate flag when constructing them, but it shouldn't be the default. In production applications where you trust how the STIX content is created, it's just unnecessary performance overhead, and you can always implement validation yourself next to any calls to DataSource/DataSink operations.

I'd love to hear others' opinions, especially if I'm missing any use cases.

@clenk
Copy link
Contributor Author

clenk commented Sep 11, 2017

I agree that users will want different settings in different parts of their application. But specifically for custom types, my thinking was that the only time you would have a problem with registering them "globally" (like we do currently) is if you need 2 with the same name but different properties. In that case you would need to be able to register them to separate environments, but I assumed that is an unlikely occurrence.

@gtback
Copy link
Contributor

gtback commented Sep 12, 2017

I was also thinking that you might want to allow custom types in some environments but not others. That does bring up the questions of how to register the same custom types in multiple environments, whether to still have a set of "global" custom types that apply to all environments, whether environments should allow nesting/inheritence, etc. It gets pretty complicated and IMO over-engineered pretty quick.

I guess what I'm saying, though, is that which custom types are supported is an environment-specific setting, but I could be wrong. The fact that the Environment.parse function doesn't depend on anything else in the Environment is, to me, a sign that it doesn't belong there, or that there should be things it depends on.

@gtback gtback merged commit 463d1e6 into master Sep 12, 2017
@gtback gtback deleted the environment branch September 12, 2017 13:59
@gtback gtback added this to the 0.3 milestone Oct 5, 2017
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

3 participants